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

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

Merged
merged 5 commits into from
Nov 9, 2024
Merged

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

merged 5 commits into from
Nov 9, 2024

Conversation

kubaflo
Copy link
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
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

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
Contributor

/azp run MAUI-UITests-public

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@IrynaDoroshenkoDev
Copy link

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

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

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
@@ -40,6 +40,9 @@ public static void UpdateCornerRadius(this ShapeableImageView platformButton, IB
.SetBottomLeftCorner(CornerFamily.Rounded, radius)
.SetBottomRightCorner(CornerFamily.Rounded, radius)
.Build();

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

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
Contributor Author

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
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

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

@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
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
Member

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

There is also #22521 which may also be related.

@mattleibow
Copy link
Member

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
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
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz mentioned this pull request Oct 15, 2024
Open
@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
Contributor

/rebase

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Nov 9, 2024

  • failing iOS controls tests not related

@PureWeen PureWeen merged commit b2d1c31 into dotnet:main Nov 9, 2024
105 of 107 checks passed
@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

@jsuarezruiz jsuarezruiz jsuarezruiz approved these changes

@PureWeen PureWeen PureWeen approved these changes

@jfversluis jfversluis Awaiting requested review from jfversluis jfversluis is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@tj-devel709 tj-devel709 Awaiting requested review from tj-devel709 tj-devel709 is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

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
8 participants
@kubaflo @jsuarezruiz @IrynaDoroshenkoDev @mattleibow @PureWeen @Eilon @samhouts @tj-devel709