Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(text): remove font padding from TextBox and TextBlock on Android #18608

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix(text): remove font padding from TextBox and TextBlock on Android #18608

wants to merge 7 commits into from

Conversation

ramezgerges
Copy link
Contributor

GitHub Issue (If applicable): closes #6528

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@github-actions github-actions bot added platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform area/automation Categorizes an issue or PR as relevant to project automation labels Oct 28, 2024
ajpinedam
ajpinedam approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@ajpinedam ajpinedam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@ramezgerges
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18608/index.html

@ramezgerges ramezgerges enabled auto-merge November 1, 2024 11:15
@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18608/index.html

@jeromelaban
Copy link
Member

@ramezgerges tests seem to have failed: https://dev.azure.com/uno-platform/Uno%20Platform/_build/results?buildId=145667&view=logs&j=3b34ba11-5751-5cc7-20e6-6c30280f122b&t=c3c46dc6-51b8-5edc-30f9-2095cc3b4c7d&l=17

@jeromelaban jeromelaban marked this pull request as draft November 6, 2024 19:31
auto-merge was automatically disabled November 6, 2024 19:31

Pull request was converted to draft

@ramezgerges ramezgerges marked this pull request as ready for review November 20, 2024 13:41
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 147901 has failed on Uno.UI - CI.

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 148353 has failed on Uno.UI - CI.

@jeromelaban
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

jeromelaban
jeromelaban reviewed Nov 27, 2024
View reviewed changes
src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.Android.cs
@@ -723,7 +724,7 @@ private void MakeLayout(int width, int maxLines = int.MaxValue)
1,
_addedSpacing = GetSpacingAdd(_paint),
_metrics,
true,
false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's validate the screenshot tests (I restarted a build so they get created properly after a recent fix).

Copy link
Member

@jeromelaban jeromelaban Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As expected, the change is showing in all the screenshot tests, could you make a comparison with the behavior found on winui? (To see if it's properly reflecting the original behavior)

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18608/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18608/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromelaban jeromelaban jeromelaban left review comments

@ajpinedam ajpinedam ajpinedam approved these changes

@MartinZikmund MartinZikmund MartinZikmund approved these changes

Assignees
No one assigned
Labels
area/automation Categorizes an issue or PR as relevant to project automation platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Android] When using custom fonts, text position in TextBox or Button doesn't render correctly
5 participants
@ramezgerges @unodevops @jeromelaban @MartinZikmund @ajpinedam