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.

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

Fixed Extra Padding on iOS buttons required when image on top or bottom #26018

Merged
merged 11 commits into from
Jan 5, 2025

Conversation

NirmalKumarYuvaraj
Copy link
Contributor

@NirmalKumarYuvaraj NirmalKumarYuvaraj commented Nov 21, 2024

Issue Details

When the image is positioned at the top or bottom with the title, extra padding is added to the left and right sides of the button.

Root Cause

When the button content layout is set to 'Top' or 'Bottom,' the button content width is recalculated based on both the title width and the image width.

Description of Change

  • Remove the logic for recalculating the button content width. If the content layout is set to 'Top' or 'Bottom,' calculate the title insets for the button based on the image width.

Validated the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Issues Fixed

Fixes #25489
Fixes #25488
Fixes #25487

Output

Before After
BeforeFix-mac Afterfix-mac

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Nov 21, 2024
@jsuarezruiz jsuarezruiz added the area-controls-button Button, ImageButton label Nov 21, 2024
@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Nov 21, 2024

/azp run

This comment was marked as off-topic.

titleInsets.Left -= imageWidth / 2;
titleInsets.Right += imageWidth / 2;
// Which makes the later math easier to follow
if (layout.Position == ButtonContentLayout.ImagePosition.Left || layout.Position == ButtonContentLayout.ImagePosition.Right)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change will impact to several tests:
image

Copy link
Member

Choose a reason for hiding this comment

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

Changes in the screenshot tests should be expected if this is working correctly so that is not necessarily a bad thing :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@tj-devel709 , @jsuarezruiz I have attached the button failure images also. Could you please check it?

@jsuarezruiz

This comment was marked as off-topic.

This comment was marked as outdated.

@NirmalKumarYuvaraj NirmalKumarYuvaraj marked this pull request as ready for review November 26, 2024 12:16
@NirmalKumarYuvaraj NirmalKumarYuvaraj requested a review from a team as a code owner November 26, 2024 12:16
@jsuarezruiz

This comment was marked as off-topic.

This comment was marked as outdated.

This comment was marked as outdated.

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@NirmalKumarYuvaraj NirmalKumarYuvaraj added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
@tj-devel709 tj-devel709 added this to the .NET 9 SR3 milestone Dec 11, 2024
@tj-devel709 tj-devel709 self-assigned this Dec 12, 2024
Copy link
Member

@tj-devel709 tj-devel709 left a comment

Choose a reason for hiding this comment

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

This PR is awesome and it is really close! The only thing I think still needs changing is when there is a border on the button. Looking at the screenshot tests for src/Controls/tests/TestCases.iOS.Tests/snapshots/ios/VerifyButtonPage4.png looks like the border is not being considered and the content of the button should be limited by the inside of the border in these case

@Shalini-Ashokan
Copy link
Contributor

This PR is awesome and it is really close! The only thing I think still needs changing is when there is a border on the button. Looking at the screenshot tests for src/Controls/tests/TestCases.iOS.Tests/snapshots/ios/VerifyButtonPage4.png looks like the border is not being considered and the content of the button should be limited by the inside of the border in these case

@tj-devel709 I have changed the logics based on the border width. Could you please review it?

@tj-devel709
Copy link
Member

It's looking better! I see that you applied the borderWidth on the left and right but is there a reason why not on the top and bottom? The image you uploaded for that test looks better except that bottom button shouldn't be cropped still

@Shalini-Ashokan
Copy link
Contributor

It's looking better! I see that you applied the borderWidth on the left and right but is there a reason why not on the top and bottom? The image you uploaded for that test looks better except that bottom button shouldn't be cropped still

@tj-devel709, When we apply a border width to the top or bottom, the image does not render properly. The issue was with the image itself. A proper image has now been committed.

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

tj-devel709
tj-devel709 previously approved these changes Dec 17, 2024
@tj-devel709
Copy link
Member

Pushed up a uitest change from the border change

@tj-devel709
Copy link
Member

/azp run

tj-devel709
tj-devel709 previously approved these changes Dec 17, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen merged commit be9e92c into dotnet:main Jan 5, 2025
104 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-button Button, ImageButton community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration
Projects
Status: Done
5 participants