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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 5 commits into from

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

PureWeen commented Oct 9, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Oct 9, 2024

/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).

@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
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

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

@jsuarezruiz jsuarezruiz jsuarezruiz left review comments

@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

At least 1 approving review is required to merge this pull request.

Assignees

@jsuarezruiz jsuarezruiz

Labels
area-controls-border Border community ✨ Community Contribution
Projects
MAUI SDK Ongoing
Status: Ready To Review
Milestone
.NET 9 SR2
7 participants
@kubaflo @jsuarezruiz @IrynaDoroshenkoDev @mattleibow @PureWeen @Eilon @tj-devel709