-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[iOS] Fixed ImageButton not honoring Padding value. #26785
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
Conversation
CGSize imageSize = platformButton.CurrentImage?.Size ?? CGSize.Empty; | ||
|
||
double contentWidth = imageSize.Width + contentEdgeInsets.Left + contentEdgeInsets.Right; | ||
double contentHeight = imageSize.Height + contentEdgeInsets.Top + contentEdgeInsets.Bottom; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thickness has a HorizontalThickness and VerticalThickness properties you could use.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Padding_Add failing
@PureWeen , The failure was due to the macOS snapshot for Padding_Add not being updated earlier. The Snapshot has been updated, and the test is expected to pass in the next CI run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix conflicts
@PureWeen, Conflicts have been resolved. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must update the macOS snapshots after: #27531
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the macOS snapshots as per your suggestion in the latest commit. Let me know if you have any further concerns. Thanks!
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way we can encapsulate this a bit into this method?
internal static CGSize SizeThatFitsImage(this UIImageView imageView, CGSize constraints) |
It looks like they are basically the same except for the padding part?
Maybe we can create an internal version of this for now that takes padding?
Thanks for the suggestion! I have refactored the code to encapsulate the logic as recommended. The updated method now includes padding values for improved reusability while preserving the existing constraints. Please review and let me know if you have any further feedback! |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some new images will need to be added to the test
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
New images have been added. |
* Fixed-ImageButton-Padding-Issue-iOS * Modified-code * Optimized-code * Added-Mac-Snapshot * fixed-conflicts * Removed-nullable-directive * Resolved-Conflicts * Modified-Code * Updated-Snapshots * Modified-Code * Modified-Snapshot --------- Co-authored-by: prakashKannanSf3972 <127308739+prakashKannanSf3972@users.noreply.github.com> Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
* Fixed-ImageButton-Padding-Issue-iOS * Modified-code * Optimized-code * Added-Mac-Snapshot * fixed-conflicts * Removed-nullable-directive * Resolved-Conflicts * Modified-Code * Updated-Snapshots * Modified-Code * Modified-Snapshot --------- Co-authored-by: prakashKannanSf3972 <127308739+prakashKannanSf3972@users.noreply.github.com> Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Issue Details
The ImageButton size calculation is incorrect. When an ImageButton has both padding and an image, its measured size does not adapt to include the padding values, causing the image to render smaller than expected.
Root Cause
The ImageButton DesiredSize is determined solely based on the ImageSize during the SizeThatFitsImage calculation. As a result, the ImageButton renders according to the image size alone, without accounting for the padding values in its measurement.
Description of Change
Updated the ImageButton to implement ICrossPlatformLayout for handling measurements, ensuring that content measurement accounts for padding values as expected, thereby improving the layout behavior.
Tested the behaviour in the following platforms
Issue Fixed
Fixes #22521
Output