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

[iOS, Android, UWP] Added padding to Button control #2426

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
9 participants
@paymicro
Copy link
Collaborator

paymicro commented Apr 11, 2018

Description of Change

Added padding to Button control

Screencasts
UWP: http://recordit.co/RO6d9sqjNR
Android: http://recordit.co/mcJqcuVA65
iOS: http://recordit.co/3MjGiRf06X

Bugs Fixed

Fixes #1702

API Changes

Added:

Thickness Padding { get; set; } //Bindable Property in Button

Behavioral Changes

User will be able to set Padding for Button.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@dnfclas

This comment has been minimized.

Copy link

dnfclas commented Apr 11, 2018

CLA assistant check
All CLA requirements met.

@samhouts samhouts added this to Ready in v3.1.0 via automation Apr 11, 2018

@rmarinho rmarinho requested review from jassmith and StephaneDelcroix Apr 12, 2018

@samhouts samhouts moved this from Ready to In Review in v3.1.0 Apr 12, 2018

@samhouts samhouts added this to In Progress in Enhancements Apr 13, 2018

@paymicro paymicro force-pushed the paymicro:fix-gh1702-padding_on_buttons branch from 947829f to 3f1006e Apr 16, 2018

@PureWeen
Copy link
Contributor

PureWeen left a comment

This is looking good. Can you also add a UITest that has 4 buttons each with a different side of the offset set? That way it can just easily be seen that offsets are all applying correctly and to the correct sides?

@@ -93,6 +93,12 @@ protected override void Build (StackLayout stackLayout)
}
);

var paddingContainer = new ViewContainer<Button> (Test.Button.Padding,

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

I think this needs to be added into the enum

@@ -260,6 +263,24 @@ void UpdateTextColor()
_textColorSwitcher?.UpdateTextColor(Control, Element.TextColor);
}

Thickness paddingDelta = new Thickness();

void UpdatePadding()

void UpdatePadding()
{
Control.Padding = new WThickness(

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

These arguments are being passed in the incorrect order.


void UpdatePadding()
{
Control?.SetPadding (

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

These arguments are being passed in the incorrect order.

@@ -268,7 +292,7 @@ void ComputeEdgeInsets(UIButton button, Button.ButtonContentLayout layout)
{
button.ImageEdgeInsets = new UIEdgeInsets(0, labelWidth + spacing, 0, -labelWidth - spacing);
button.TitleEdgeInsets = new UIEdgeInsets(0, -imageWidth - spacing, 0, imageWidth + spacing);
button.ContentEdgeInsets = new UIEdgeInsets(0, 2 * spacing, 0, 2 * spacing);
UpdateContentEdge (button, new Thickness (0, 2 * spacing, 0, 2 * spacing));

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

Thickness is Left, top, right, bottom
UIEdgeInset is Top, right, bottom, left

So I think all these places where you replaced UIEdgeInsets with Thickness will need to account for this

@PureWeen
Copy link
Contributor

PureWeen left a comment

Looking good just some quirks in Android . Here's what I'm using locally to test if you want to use it for a UI Test

StackLayout layout = null;
			layout = new StackLayout()
			{
				Children =
					{
						new Button()
						{
							Image = "coffee.png",
							BackgroundColor = Color.Green,
							Text = "Do I have no padding? I should have no padding. Fail me if I don't"
						},
						new Button()
						{
							Image = "coffee.png",
							BackgroundColor = Color.Green,
							Padding = new Thickness(100, 0, 0, 0),
							Text = "Do I have left padding? I should have left padding. Fail me if I don't"
						},
						new Button()
						{
							Image = "coffee.png",
							BackgroundColor = Color.Green,
							Padding = new Thickness(0, 100, 0, 0),
							Text = "Do I have top padding? I should have top padding. Fail me if I don't"
						},
						new Button()
						{
							Image = "coffee.png",
							BackgroundColor = Color.Green,
							Padding = new Thickness(0, 0, 100, 0),
							Text = "Do I have right padding? I should have right padding. Fail me if I don't"
						},
						new Button()
						{
							Image = "coffee.png",
							BackgroundColor = Color.Green,
							Padding = new Thickness(0, 0, 0, 100),
							Text = "Do I have bottom padding? I should have bottom padding. Fail me if I don't"
						},
						new Button()
						{
							Text = "Clear text make sure image padding resets correctly",
							Command = new Command(() =>
							{
								foreach(Button button in layout.Children.Take(5))
								{
									button.Text = String.Empty;
								}
							})
						}
					}
			};

			return new ContentPage()
			{
				Content = layout
			};
@@ -277,6 +280,24 @@ void UpdateTextColor()
_textColorSwitcher?.UpdateTextColor(Control, Element.TextColor);
}

Thickness paddingDelta = new Thickness ();

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

Move to the top of the class.

@@ -470,6 +475,24 @@ void UpdateTextColor()
_textColorSwitcher.Value.UpdateTextColor(this, Button.TextColor);
}

Thickness paddingDelta = new Thickness ();

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

Move to the top of the class.

@@ -260,6 +263,24 @@ void UpdateTextColor()
_textColorSwitcher?.UpdateTextColor(Control, Element.TextColor);
}

Thickness paddingDelta = new Thickness();

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

Move to the top of the class.

@@ -227,14 +230,35 @@ void UpdateTextColor()
}
}

UIEdgeInsets paddingDelta = new UIEdgeInsets();

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

Move to the top of the class.


void UpdatePadding()
{
Control?.SetPadding(

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

These need to be converted ToPixels

Control?.SetPadding(
				(int)(Context.ToPixels(Element.Padding.Left) + paddingDelta.Left),
				(int)(Context.ToPixels(Element.Padding.Top) + paddingDelta.Top),
				(int)(Context.ToPixels(Element.Padding.Right) + paddingDelta.Right),
				(int)(Context.ToPixels(Element.Padding.Bottom) + paddingDelta.Bottom)
			);

void IPaddingElement.OnPaddingPropertyChanged (Thickness oldValue, Thickness newValue)
{
UpdatePadding (newValue);

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

Invalidate Measure
When the padding is changed the measure on the button is going to change so that needs to Invalidate the Measure on the button. You can see the issue with this if you set the padding after the button has renderered vs before. If you don't invalidate the measure then it would look different in each case because the height/width won't adjust to the new padding

Example

	void IFontElement.OnFontFamilyChanged(string oldValue, string newValue) =>
			InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged);
@@ -294,7 +299,7 @@ protected override void OnLayout(bool changed, int l, int t, int r, int b)
// we just need to adjust the padding so it centers vertically
int diff = (b - t - _imageHeight) / 2;
diff = Math.Max(diff, 0);
SetPadding(0, diff, 0, -diff);
UpdateContentEdge(new Thickness (0, diff, 0, -diff));

This comment has been minimized.

@PureWeen

PureWeen May 2, 2018

Contributor

I think the math here is going to need to account for the padding when it sets the content edges. If you create a Button with bottom padding and an image

new Button()
{
     Image = "coffee.png",
     BackgroundColor = Color.Green,
     Padding = new Thickness(0, 0, 0, 100)
}

Then on android it looks like it takes that padding and evens it out between top and bottom ultimately centering the image instead of giving it bottom padding. If you give it top padding and not bottom padding then it starts to push the image out of the button view

I have the code below and it mostly seems to work. I'm a little curious about some edge case sizing not causing an even padding shift though

	protected override void OnLayout(bool changed, int l, int t, int r, int b)
		{
			if (_imageHeight > -1)
			{	
				var bottomPadding = Context.ToPixels(Element.Padding.Bottom);
				var topPadding = Context.ToPixels(Element.Padding.Top);
				// We've got an image (and no text); it's already centered horizontally,
				// we just need to adjust the padding so it centers vertically
				var diff = ((b - bottomPadding - topPadding) - t - _imageHeight) / 2;
				diff = Math.Max(diff, 0);
				UpdateContentEdge(new Thickness(0, diff, 0, -diff));
			}

			base.OnLayout(changed, l, t, r, b);
		}

This comment has been minimized.

@paymicro

paymicro May 3, 2018

Author Collaborator

I think if a padding element is specified at the bottom or top then you need to ignore the diff padding "button" with the image.
I think it is not necessary to increase the padding, if it was specified with the user.

@paymicro paymicro force-pushed the paymicro:fix-gh1702-padding_on_buttons branch from 32de61c to 4008007 May 3, 2018

@@ -55,13 +56,13 @@ public override SizeRequest GetDesiredSize(int widthConstraint, int heightConstr

protected override void OnLayout(bool changed, int l, int t, int r, int b)
{
if (_imageHeight > -1)
if (_imageHeight > -1 && Element.Padding.Bottom == 0 && Element.Padding.Top == 0)

This comment has been minimized.

@PureWeen

PureWeen May 4, 2018

Contributor

So I don't think just opting out like this quite works. I've had success with this

protected override void OnLayout(bool changed, int l, int t, int r, int b)
		{
			if (_imageHeight > -1)
			{
				// We've got an image (and no text); it's already centered horizontally,
				// we just need to adjust the padding so it centers vertically
				var diff = ((b - Context.ToPixels(Element.Padding.Bottom + Element.Padding.Top))  - t - _imageHeight) / 2;
				diff = Math.Max(diff, 0);
				UpdateContentEdge(new Thickness(0, diff, 0, -diff));

			}
			else
			{
				UpdateContentEdge();
			}

			base.OnLayout(changed, l, t, r, b);
		}

Here's some tester code to try. Using the code you have if I start adding top/bottom padding it causes it to jump to jump as you add padding.

StackLayout layout = null;
			layout = new StackLayout()
			{
				Children =
					{
						new Button()
						{
							Image = "coffee.png",
							BackgroundColor = Color.Green,
							Padding = new Thickness(),
							HeightRequest = 400,
						},
						new Button()
						{
							Text = "Bottom",
							Command = new Command(() =>
							{
								var button = layout.Children[0] as Button;
								button.Padding = new Thickness(0, button.Padding.Top, 0,button.Padding.Bottom + 10);
								if(!String.IsNullOrWhiteSpace(button.Text))
									button.Text = "Top: " + button.Padding.Top.ToString() + " Bottom: " + button.Padding.Bottom.ToString();
							})
						},
						new Button()
						{
							Text = "Top",
							Command = new Command(() =>
							{
								var button = layout.Children[0] as Button;
								button.Padding = new Thickness(0, button.Padding.Top + 10, 0, button.Padding.Bottom);
								if(!String.IsNullOrWhiteSpace(button.Text))
									button.Text = "Top: " + button.Padding.Top.ToString() + " Bottom: " + button.Padding.Bottom.ToString();
							})
						},
						new Button()
						{
							Text = "Reset",
							Command = new Command(() =>
							{
								var button = layout.Children[0] as Button;
								button.Padding = new Thickness(0, 0, 0,0);
							})
						},
						new Button()
						{
							Text = "Set Text",
							Command = new Command(() =>
							{
								var button = layout.Children[0] as Button;
								button.Text = "some text";
							})
						},
						new Button()
						{
							Text = "Reset Text",
							Command = new Command(() =>
							{
								var button = layout.Children[0] as Button;
								button.Text = String.Empty;
							})
						},
					}
			};

			return new ContentPage()
			{
				Content = layout
			};
@@ -239,6 +239,7 @@
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla59580.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Effects\AttachedStateEffect.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Effects\AttachedStateEffectList.cs" />
<Compile Include="$(MSBuildThisFileDirectory)GitHub1702.cs" />

This comment has been minimized.

@PureWeen

PureWeen May 4, 2018

Contributor

Rename to Issue1702.cs

This comment has been minimized.

@paymicro

paymicro May 4, 2018

Author Collaborator

I noticed that in Issue#### classes some other github number is specified and different description.
For example Issue1461.cs created almost 2 years ago

[Issue (IssueTracker.Github, 1461, "1461 - (Popover in Portrait and Landscape)", PlatformAffected.iOS)]
public class Issue1461 : TestContentPage

And Github1461.cs created a couple of months ago
[Issue(IssueTracker.Github, 1461, "Cannot change TimePicker format", PlatformAffected.Android)]
public class Github1461 : TestContentPage // or TestMasterDetailPage, etc ...

Github1461.cs is correct for this repository #1461

I was confused. Is renaming to Issue#### still valid?

This comment has been minimized.

@PureWeen

PureWeen May 4, 2018

Contributor

Yea :-/ Right now there's a mix of numbers that relate to the old github repo before the project went public. For now we're still just creating the Issues as Issue#### when there isn't a number collision

InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged);
}

protected virtual void UpdatePadding (Thickness newValue)

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

this isn't necessary. subclass of Button have other ways of detecting Padding changes.

@paymicro paymicro force-pushed the paymicro:fix-gh1702-padding_on_buttons branch from 89e8f72 to 0e98a44 May 7, 2018

@samhouts samhouts added this to In Review in vNext (3.6.0) May 7, 2018

@samhouts samhouts removed this from In Review in v3.1.0 May 7, 2018

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented May 8, 2018

@paymicro can you rebase this against master so we can get those UI Tests running with some green checks

@rmarinho
Copy link
Member

rmarinho left a comment

Failing UITests

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented May 9, 2018

Yea they are all from the Scale Matrix error. Once this is rebased with the fix from a few hours ago those should pass.

@paymicro paymicro force-pushed the paymicro:fix-gh1702-padding_on_buttons branch from 0e98a44 to 47b4170 May 10, 2018

@paymicro

This comment has been minimized.

Copy link
Collaborator Author

paymicro commented May 10, 2018

rebase and squash completed =)

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented May 10, 2018

build --uitests

@rmarinho rmarinho merged commit 53a0551 into xamarin:master May 14, 2018

10 of 13 checks passed

VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: Xamarin Forms (PR Builds) PR-2426 - (1678605) succeeded
Details
VSTS: Xamarin Forms OSX PR-2426 - (1678633) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-2426 - (1678622) succeeded
Details
VSTS: iOS10 Validation UITests Finished
Details
VSTS: iOS11 Validation UITests Finished
Details
VSTS: iOS9 Validation UITests Finished
Details
license/cla All CLA requirements met.
Details

Enhancements automation moved this from In Progress to Done May 14, 2018

vNext (3.6.0) automation moved this from In Review to Done May 14, 2018

@paymicro paymicro deleted the paymicro:fix-gh1702-padding_on_buttons branch May 15, 2018

@samhouts samhouts removed this from Done in Enhancements Jun 12, 2018

@samhouts samhouts added this to the 3.2.0 milestone Jun 26, 2018

@samhouts samhouts removed this from Done in vNext (3.6.0) Jun 26, 2018

@samhouts samhouts added this to Done in v3.2.0 Jun 26, 2018

@samhouts samhouts modified the milestone: 3.2.0 Sep 12, 2018

@czuvich

This comment has been minimized.

Copy link

czuvich commented Sep 25, 2018

I think this change adversely impacted buttons with an Image and Text. It looks like the Image and Text now hug the boundaries of the button. I've tried changing the ContentLayout to different options, but it has no effect (at least on Android). I'll do some more testing and submit a bug report.

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