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

Implement Padding property in ImageButtonHandler #4665

Merged
merged 14 commits into from
Mar 28, 2022
Merged

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Implement Padding property in ImageButton.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

Does this PR touch anything that might affect accessibility?

  • No

@jsuarezruiz jsuarezruiz added legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor area-controls-button Button, ImageButton labels Feb 14, 2022
@rmarinho rmarinho requested a review from PureWeen February 14, 2022 14:07
@PureWeen PureWeen requested a review from mattleibow February 14, 2022 14:59
@Redth
Copy link
Member

Redth commented Feb 16, 2022

@jsuarezruiz I don't see the padding value influencing MacCatalyst at all in the ImageButton gallery page.

@J-Swift
Copy link
Contributor

J-Swift commented Mar 4, 2022

@jsuarezruiz I happened to be looking into this because I noticed it wasn't implemented yet, and noticed that your Android implementation won't work as expected due to an Android Framework bug which pads the entire button rather than just the content area (material-components/material-components-android#2063).

This is what that looks like:
Screen Shot 2022-03-03 at 10 11 32 PM

I propose the following for the Android implementation:

public static void UpdatePadding(this ShapeableImageView platformButton, IImageButton imageButton)
{
		var pad = imageButton.Padding;

		// NOTE(jpr): post on handler to get around an Android Framework bug.
		// see: https://github.com/material-components/material-components-android/issues/2063
		platformButton.Post(() =>
		{
				platformButton.SetContentPadding(
					(int)platformButton.Context.ToPixels(pad.Left),
					(int)platformButton.Context.ToPixels(pad.Top),
					(int)platformButton.Context.ToPixels(pad.Right),
					(int)platformButton.Context.ToPixels(pad.Bottom)
				);
		});
}

which results in this

Screen Shot 2022-03-03 at 10 13 53 PM

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

The padding may be wrong and there might be an android bug that we need to work around.

@jsuarezruiz
Copy link
Contributor Author

@jsuarezruiz I happened to be looking into this because I noticed it wasn't implemented yet, and noticed that your Android implementation won't work as expected due to an Android Framework bug which pads the entire button rather than just the content area (material-components/material-components-android#2063).

This is what that looks like: Screen Shot 2022-03-03 at 10 11 32 PM

I propose the following for the Android implementation:

public static void UpdatePadding(this ShapeableImageView platformButton, IImageButton imageButton)
{
		var pad = imageButton.Padding;

		// NOTE(jpr): post on handler to get around an Android Framework bug.
		// see: https://github.com/material-components/material-components-android/issues/2063
		platformButton.Post(() =>
		{
				platformButton.SetContentPadding(
					(int)platformButton.Context.ToPixels(pad.Left),
					(int)platformButton.Context.ToPixels(pad.Top),
					(int)platformButton.Context.ToPixels(pad.Right),
					(int)platformButton.Context.ToPixels(pad.Bottom)
				);
		});
}

which results in this

Screen Shot 2022-03-03 at 10 13 53 PM

Used the Jimmy workaround. Thanks for include the issue.

@rmarinho rmarinho self-requested a review March 25, 2022 11:00
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

@Redth
Copy link
Member

Redth commented Mar 28, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Redth Redth merged commit a270ebd into main Mar 28, 2022
@Redth Redth deleted the imagebutton-padding branch March 28, 2022 18:31
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
@samhouts samhouts added the fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-button Button, ImageButton fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants