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

[Enhancement] Padding on Buttons #1702

Closed
samhouts opened this Issue Jan 26, 2018 · 24 comments

Comments

Projects
10 participants
@samhouts
Member

samhouts commented Jan 26, 2018

Rationale

Buttons do not currently have a Padding property, forcing some users to create hacks, custom renderers, or effects to achieve the desired layout.

Implementation

Add bindable property to Button & make Button implement IPaddingElement:

public static readonly BindableProperty PaddingProperty = PaddingElement.PaddingProperty;

Renderers will need to be updated accordingly.

Expected Result

Android

  • Button has no padding by default.
  • Altering the Padding on a Button will change the size of the Button relative to the position of the Text.

iOS

See Android

UWP

See Android

Implications for CSS

Button will now support padding.

Backward Compatibility

Since we are adding new properties, there should be no compatibility problems as long as:

  1. We ensure that an Button's padding defaults to 0, as is the expected behavior now,
  2. We ensure that any Effects or Custom Renderers that may have been created to produce this behavior take precedence over any values that we set, to the best of our ability.
  3. We ensure that this does not conflict with the new TextAlignment properties on Buttons.

Third party renderers will need to be updated to ensure that this functionality is officially supported.

Difficulty : Medium

@pauldipietro pauldipietro added this to New in Triage Jan 26, 2018

@jassmith jassmith added this to Ready for Implementation in Enhancements Jan 26, 2018

@llevera

This comment has been minimized.

llevera commented Jan 29, 2018

I'm keen to take this on, having recently been in need of this and am aware of common work arounds to check the compatibility of coexisting with

@hartez hartez removed this from New in Triage Jan 29, 2018

@llevera

This comment has been minimized.

llevera commented Jan 31, 2018

@davidortinau Just confirming I am looking at this one

@llevera

This comment has been minimized.

llevera commented Jan 31, 2018

This appears an excellent basis for this change https://stackoverflow.com/a/36067371

@andreinitescu

This comment has been minimized.

Contributor

andreinitescu commented Jan 31, 2018

I wish more controls have padding...

@davidortinau davidortinau self-assigned this Feb 1, 2018

@davidortinau davidortinau moved this from Ready for Implementation to In Progress in Enhancements Feb 1, 2018

@jassmith

This comment has been minimized.

Contributor

jassmith commented Feb 17, 2018

@llevera just confirming you are still looking into this :) If not we can move it back to the ready for work column

@arevellfraedom

This comment has been minimized.

arevellfraedom commented Feb 17, 2018

Yep, still working on it (ableit sporadically). First PR shouldn't be far away

@jassmith

This comment has been minimized.

Contributor

jassmith commented Feb 17, 2018

Awesome thanks!

@jerone

This comment has been minimized.

jerone commented Feb 18, 2018

Some logic for elements with padding has already been setup up in #1276, where it is implemented in Frame, Layout and Page.

Wondering if IPaddingElement should/could be extended to all View/VisualElement's... to be in sync with Margin.

@jassmith

This comment has been minimized.

Contributor

jassmith commented Feb 18, 2018

Not everything needs a padding. What does the padding a Switch do? You mgiht think it makes sense to just generate blank space around the switch, but thats more of a Margin job. Margins work externally to the control, but there is nothing internal to a switch on which the Padding could work...

@andreinitescu

This comment has been minimized.

Contributor

andreinitescu commented Feb 18, 2018

@jassmith The same thing which UWP's Padding property does on ToggleSwitch. I think the best explanation is in the remarks of the Control's Padding property:

Each control might apply this property differently based on its visual template. This property only affects a control whose template uses the Padding property as a parameter. On other controls, this property has no effect

Of course, Xamarin has no template for Switch but I think the idea should be similar. Padding on some Xamarin Forms control could just not do anything.

I wish in Xamarin Forms Padding property was on View. I still remember the dark old days when there was no Margin on controls and everyone (I swear, not me!) was wrapping controls in a ContentView.

@llevera

This comment has been minimized.

llevera commented Feb 18, 2018

This is a little devil's advocate, but surely the padding of any control would be apparent if a background colour is set (and can be applied in a reasonable way)?

@llevera

This comment has been minimized.

llevera commented Feb 18, 2018

BTW, this change is relatively simple except for the fact that image buttons are implemented by manipulating the insets/paddings (in non-trivial ways, esp for Android). So doing this change while maintaining that functionality is the hard bit.

@andreinitescu

This comment has been minimized.

Contributor

andreinitescu commented Feb 18, 2018

By the way, this guy things the same, albeit it's a very old article back in the dark days when Margin didn't exist on View (shrugging) http://blog.tpcware.com/2014/09/xamarin-xaml-vs-microsoft-xaml-the-devil-is-in-the-details/

@llevera

This comment has been minimized.

llevera commented Mar 2, 2018

@jassmith It's time I withdrew from this issue. I'm not going to be able to find the time to finish this.

@weitzhandler

This comment has been minimized.

weitzhandler commented Apr 1, 2018

Regarding Button in Android, this is a total annoyance and a must - there is no way to get rid of the annoying padding on Android. See #2220.
But I'd wish Padding to be added to View rather than just to Button.

I also opened a UserVoice suggesion before discovering this post.

@samhouts samhouts moved this from In Progress to Ready for Implementation in Enhancements Apr 2, 2018

paymicro added a commit to paymicro/Xamarin.Forms that referenced this issue Apr 10, 2018

paymicro added a commit to paymicro/Xamarin.Forms that referenced this issue Apr 10, 2018

@weitzhandler

This comment has been minimized.

weitzhandler commented Apr 10, 2018

@paymicro I can add the required work to UWP if you want me to.

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

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

@samhouts samhouts moved this from Ready for Implementation to In Progress in Enhancements Apr 12, 2018

@samhouts samhouts added this to In Progress in vNext+1 (master) May 7, 2018

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

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

rmarinho added a commit that referenced this issue May 14, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) May 14, 2018

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

@samhouts samhouts removed this from Done in vNext+1 (master) Jun 26, 2018

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

@weitzhandler

This comment has been minimized.

weitzhandler commented Jul 19, 2018

Thank you @paymicro, @samhouts, and @rmarinho you guys are awesome ❤️

@Goldstrike

This comment has been minimized.

Goldstrike commented Jul 23, 2018

Does this work in the latest published version?
I've tested it but I still not get a Padding parameter...

@TFreudi1

This comment has been minimized.

TFreudi1 commented Sep 1, 2018

It is still not working, please Reopen it

VS 2017 15.8.2

gives
Unhandled Exception:

Xamarin.Forms.Xaml.XamlParseException: Position 24:21. Cannot assign property "Padding": Property does not exists, or is not assignable, or mismatching type between value and property

@weitzhandler

This comment has been minimized.

weitzhandler commented Sep 1, 2018

  1. U sure you updated Xamarin.Forms package in your projects?
  2. Try cleaning solution and obj folders before rebuild
@TFreudi1

This comment has been minimized.

TFreudi1 commented Sep 2, 2018

I did all this, Xamarin.Forms Version is 3.1.0.697729, but there is no "Padding" parameter in Button.

@TFreudi1

This comment has been minimized.

TFreudi1 commented Sep 2, 2018

@arevellfraedom

This comment has been minimized.

arevellfraedom commented Sep 2, 2018

This issue was closed, not merged. The actual change that implemented this is #2426 which will be included in 3.2

@TFreudi1

This comment has been minimized.

TFreudi1 commented Sep 2, 2018

Well OK, in Xamarin.Forms 3.2 Pre it works like charme, thank you guys, good work.

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