Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Android] Improve default Button shadow & size appearance #3251

Closed
wants to merge 5 commits into from

Conversation

samhouts
Copy link
Member

@samhouts samhouts commented Jul 9, 2018

Description of Change

When using the DefaultPadding and DefaultShadow platform specific properties, Button size and shadow now more closely match the default Android size and shadow. Note that this is not intended to be a pixel perfect match.

#2664 Before >> After

2664

#1436 Before >> After

1436

WARNING: This is a visual breaking change from 2.5+.

Issues Resolved

API Changes

None

Platforms Affected

  • Android

Behavioral/Visual Changes

  • [Android] Buttons now have a slightly different shadow radius that more closely mimics Android's default Button image.
  • [Android] Button shadow color now defaults to a light gray instead of a shade of the Button color.
  • [Android] Button border is drawn outside of the Button instead of on top of it and can increase the size of the Button.
  • [Android] Button width is slightly increased.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@samhouts samhouts added the breaking Changes behavior or appearance label Jul 9, 2018
@samhouts samhouts requested review from jassmith and hartez July 16, 2018 23:24
float shadowDy = useDefaultShadow ? 4 : _nativeButton.ShadowDy;
AColor shadowColor = useDefaultShadow ? _backgroundDrawable.PressedBackgroundColor.ToAndroid() : _nativeButton.ShadowColor;
float shadowDy = useDefaultShadow ? 1 : _nativeButton.ShadowDy;
AColor shadowColor = useDefaultShadow ? Color.FromHex("#c4c4c4").ToAndroid() : _nativeButton.ShadowColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a reference for this color code?

@@ -36,6 +36,9 @@ float PaddingTop
set { _paddingTop = value; }
}

double BorderWidth => Math.Max(Button.IsSet(Button.BorderWidthProperty) ? Button.BorderWidth : .25, 0);
Color BorderColor => Button.IsSet(Button.BorderColorProperty) && Button.BorderColor != Color.Default ? Button.BorderColor : Color.FromHex("#c5c5c5");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - where do these colors come from? Is there a guide or source code we can point to?

@samhouts
Copy link
Member Author

samhouts commented Oct 3, 2018

build --uitests

@samhouts samhouts added e/7 🕖 7 i/low Has trivial workaround; affects very few users p/Android t/bug 🐛 labels Nov 2, 2018
@samhouts samhouts removed the request for review from jassmith November 21, 2018 22:43
@rmarinho rmarinho self-requested a review December 7, 2018 00:58
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.

Every thing looks good. While testing the ButtonGallery i just found a little difference, trying to understand from where:
Border radius before
screenshot_1544187056

Border radius after
screenshot_1544187524

This one is expected right and the most breaking change BorderWidth

Before:
screenshot_1544187087

After:
screenshot_1544187533


RectF rect = new RectF(0, 0, width, height);
rect.Inset(inset + PaddingLeft, inset + PaddingTop);
Copy link
Member

@rmarinho rmarinho Dec 7, 2018

Choose a reason for hiding this comment

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

@samhouts after some testing , adding this back and subtracting the inset above that we remove, fix the issue with the corner radius that I noticed in my review, at the same time it makes the border width look bigger basically looking as before

Copy link
Member

@rmarinho rmarinho Dec 7, 2018

Choose a reason for hiding this comment

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

More testing seems that adding what we had with inset still makes #2664 look the same. Maybe we should pull that in ? At the same time I like the border better without the inset :/

(With inset changes back in)

screenshot_1544188838

1436 (With inset changes back in)
1st button seems smaller

screenshot_1544188966

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing the same thing as @rmarinho on this one
I've been playing with this for a little bit and here's where I am so far

https://gist.github.com/PureWeen/855e7acdffe2087553a75881f95d23f6
Basically I changed the insets a bit because I wasn't quite sure on the intent of the math in the PR

I inset the background just based on padding and shadow
And then I inset the Outline the same but then plus half the borderwidth which just shrinks it down so the outer edge of the stroke lines up with the background

The effect of this is that if you set a really wide stroke you'll still see a shadow

@mattleibow
Copy link
Contributor

This is probably something that we want to do: #4185

@samhouts samhouts changed the base branch from master to 4.0.0 April 22, 2019 22:30
Co-Authored-By: Shane Neuville <pureween@users.noreply.github.com>
@samhouts
Copy link
Member Author

I think this PR introduces more problems than it solves. Closing for now.

@samhouts samhouts closed this Apr 29, 2019
@rmarinho rmarinho deleted the fix-2664 branch November 14, 2019 12:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking Changes behavior or appearance DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. e/7 🕖 7 i/low Has trivial workaround; affects very few users p/Android t/bug 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants