Skip to content

ImageButton border (BorderWidth) overlaps the image instead of adding space - fix#21259

Merged
PureWeen merged 5 commits intodotnet:mainfrom
kubaflo:fix-20801
Nov 9, 2024
Merged

ImageButton border (BorderWidth) overlaps the image instead of adding space - fix#21259
PureWeen merged 5 commits intodotnet:mainfrom
kubaflo:fix-20801

Conversation

@kubaflo
Copy link
Copy Markdown
Contributor

@kubaflo kubaflo commented Mar 18, 2024 •
edited
Loading

Issues Fixed

Fixes #24856
Fixes #22520

Before After

@kubaflo kubaflo requested a review from a team as a code owner March 18, 2024 00:54
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Mar 18, 2024
jsuarezruiz
jsuarezruiz reviewed Mar 18, 2024
View reviewed changes
@jsuarezruiz
Copy link
Copy Markdown
Contributor

jsuarezruiz commented Mar 18, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 18, 2024

Azure Pipelines successfully started running 3 pipeline(s).

@Eilon Eilon added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Mar 28, 2024
@jsuarezruiz
Copy link
Copy Markdown
Contributor

jsuarezruiz commented Apr 16, 2024

/azp run MAUI-UITests-public

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 16, 2024

Azure Pipelines successfully started running 1 pipeline(s).

@IrynaDoroshenkoDev
Copy link
Copy Markdown

IrynaDoroshenkoDev commented Apr 19, 2024

Thank you Kuba! I used your fix in my handler and it works perfectly!

@jsuarezruiz
Copy link
Copy Markdown
Contributor

jsuarezruiz commented May 10, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented May 10, 2024

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Copy Markdown
Contributor

jsuarezruiz commented May 10, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented May 10, 2024

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz
jsuarezruiz reviewed May 13, 2024
View reviewed changes
src/Core/src/Platform/Android/ImageButtonExtensions.cs Outdated
.Build();

int contentPadding = (int)(radius / 2);
platformButton.SetContentPadding(contentPadding, contentPadding, contentPadding, contentPadding);
Copy link
Copy Markdown
Contributor

@jsuarezruiz jsuarezruiz May 13, 2024

Choose a reason for hiding this comment

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

The changes are impacting the test Issue18242Test clipping the image:
image

Copy link
Copy Markdown
Contributor Author

@kubaflo kubaflo May 14, 2024

Choose a reason for hiding this comment

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

Hmmm I'm not sure what is wrong here and if it is a good thing or not

@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 13, 2024
@jsuarezruiz
Copy link
Copy Markdown
Contributor

jsuarezruiz commented May 17, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented May 17, 2024

Azure Pipelines successfully started running 3 pipeline(s).

@espenrl espenrl mentioned this pull request May 24, 2024
Closed
@mattleibow
Copy link
Copy Markdown
Member

mattleibow commented May 30, 2024

@kubaflo I like these tests but I had to re-work the whole background/border/padding parts of the ImageButton because we lost the ripple at some point: #22298

I think I fixed the issue and my tests seemed to pass. But, not sure if you rebase but keep the tests if you get the same result.

@kubaflo kubaflo changed the title [Android] ImageButton Border does not fit to CornerRadius - fix [Android] ImageButton Border does not fit to CornerRadius -UiTest May 30, 2024
@kubaflo
Copy link
Copy Markdown
Contributor Author

kubaflo commented May 31, 2024 •
edited
Loading

@kubaflo I like these tests but I had to re-work the whole background/border/padding parts of the ImageButton because we lost the ripple at some point: #22298

I think I fixed the issue and my tests seemed to pass. But, not sure if you rebase but keep the tests if you get the same result.

@mattleibow I've merged the main branch. I've also added one commit because when I tested with the main branch the Android didn't work IMO. Please have a look:

iOS Android

After my commit this is how the Android looks like:

But it might not be a correct fix, so please have a look at it 😊

@mattleibow
Copy link
Copy Markdown
Member

mattleibow commented May 31, 2024

Hmmm, this looks like the #22520 issue so you fixed that :)

There is also #22521 which may also be related.

@mattleibow
Copy link
Copy Markdown
Member

mattleibow commented May 31, 2024

This is nice that it fixes Android, I think iOS also has the same issue. How easy is it to fix ios and then we merge? If things are complicated, as buttons on iOS are, then we can do a separate PR.

I know TJ has been working on normal buttons and that has been a pain - especially since we can't derive from UIButton.

But no need to delay this fix for Android just because ios is being difficult. But, if there is basic padding math then it may be worth just doing it all in one go.

@kubaflo
Copy link
Copy Markdown
Contributor Author

kubaflo commented May 31, 2024 •
edited by mattleibow
Loading

This is nice that it fixes Android, I think iOS also has the same issue. How easy is it to fix ios and then we merge? If things are complicated, as buttons on iOS are, then we can do a separate PR.

I know TJ has been working on normal buttons and that has been a pain - especially since we can't derive from UIButton.

But no need to delay this fix for Android just because ios is being difficult. But, if there is basic padding math then it may be worth just doing it all in one go.

@mattleibow Yeah, I've added a commit that fixes iOS

@mattleibow
Copy link
Copy Markdown
Member

mattleibow commented May 31, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented May 31, 2024

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Copy Markdown
Member

PureWeen commented Oct 11, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Oct 11, 2024

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Copy Markdown
Member

PureWeen commented Oct 12, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Oct 12, 2024

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz mentioned this pull request Oct 15, 2024
Closed
@PureWeen PureWeen added this to the .NET 9 SR1 milestone Oct 23, 2024
@tj-devel709 tj-devel709 self-assigned this Oct 31, 2024
@jsuarezruiz jsuarezruiz self-assigned this Nov 4, 2024
@PureWeen PureWeen modified the milestones: .NET 9 SR1, .NET 9 SR2 Nov 5, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 7, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 7, 2024
@jsuarezruiz
Copy link
Copy Markdown
Contributor

jsuarezruiz commented Nov 7, 2024

/rebase

@jsuarezruiz
Copy link
Copy Markdown
Contributor

jsuarezruiz commented Nov 7, 2024

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Nov 7, 2024

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Copy Markdown
Member

PureWeen commented Nov 9, 2024

  • failing iOS controls tests not related

@PureWeen PureWeen merged commit b2d1c31 into dotnet:main Nov 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2024
@samhouts samhouts added fixed-in-9.0.21 fixed-in-net8.0-nightly This may be available in a nightly release! labels Dec 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@PureWeen PureWeen PureWeen approved these changes

@jfversluis jfversluis Awaiting requested review from jfversluis jfversluis was automatically assigned from dotnet/dotnet-maui-reviewers

@tj-devel709 tj-devel709 Awaiting requested review from tj-devel709 tj-devel709 was automatically assigned from dotnet/dotnet-maui-reviewers

+1 more reviewer

@jsuarezruiz jsuarezruiz jsuarezruiz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jsuarezruiz jsuarezruiz

Labels

area-controls-border Border community ✨ Community Contribution fixed-in-9.0.21 fixed-in-net8.0-nightly This may be available in a nightly release!

Projects

MAUI SDK Ongoing
Status: Done

Milestone

.NET 9 SR2

Development

Successfully merging this pull request may close these issues.

Android ImageButton Aspect="AspectFit" not display correctly ImageButton border (BorderWidth) overlaps the image instead of adding space

8 participants

@kubaflo @jsuarezruiz @IrynaDoroshenkoDev @mattleibow @PureWeen @Eilon @samhouts @tj-devel709