[Amsterdam] Updating button styles #2874

daveyholler · 2020-02-19T18:50:54Z

Summary

Updates button styles.

Buttons that were hollow in appearance in the default EUI theme are now a subdued version of their parent color.
Shadows have been removed
Button text is now a heavier 600 weight
Border radius (except on small buttons) has been increased to a calculated value of 6px

Details

There's some additional files here. Lemme break it down:

mixins/_button.scss: Modifies current theme's mixins/_button.scss file
variables/_buttons.scss: Modifies current theme's variables/_buttons.scss file
overrides/*.scss: Modifies the EuiButton* component stylesheet. This is intended as a temporary stop gap until Amsterdam is the default theme. Certain elements like shadows and borders exist in the component code that are to be removed in Amsterdam.

Consumers

The default un-filled/hollow buttons in EUI's current theme have been replaced by a filled version that use a subdued variant of the button's assigned color. In most use cases there should be no need to update or modify your existing UIs. For additional information on how/when to use certain button styles, please refer to our Button Guidelines

Follow Ups

Button group styles will be updated in a follow up PR to enhance clarity between default and toggled states.

Dev

In order to support theme overrides that require more than just SASS variable changes, and to avoid duplicating lots of code, several additional steps were taken for the button:

Broke euiButton() mixin down to smaller mixins for easier reuse of base styles
The overrides/ folder sits outside of global_styling/ and is imported after component imports
- Then you only need to override what is different from the default theme and not the entire markup

Disabled styles

There was a lot of manual configuring to create the disabled styles for the different types of buttons. I ended up creating a new variable (in buttons only) called $euiButtonColorDisabledTextColor that just calculates the contrast to the 2.0 necessary for disabled elements. This is then used in text portions of disabled buttons that don't have a background. I also then use the same calculation for fill buttons to change the text color to achieve a 2.0 contrast which does mean changing the text color from white to a dark gray:

Before

After

Dark mode

But this is a much more sustainable approach when it comes to theming as I was able to remove a lot of overrides for Amsterdam.

Ghost buttons

This PR also has a fix for the current theme's ghost button disabled styles.

Deprecations #1469

`text` color EuiButton (and alts)

We should really deprecate the text color for buttons as it looks too similar to disabled buttons. We can keep it for EuiButtonIcon and EuiButtonEmpty, but the basic buttons shouldn't allow it.

`disabled` color EuiButton (and alts)

It doesn't quite make sense to have a disabled color for buttons. Instead we should rely solely on the :disabled property.

Checklist

Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
Checked in ~~IE11~~ Edge and Firefox
- [ ] Props have proper autodocs
Added documentation examples
- [ ] Added or updated jest tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

johnbarrierwilson · 2020-02-19T19:23:00Z

Is that screenshot using the current 16px base font size? Will it shrink a little when we switch to 14px? Just curious. It doesn't look bad right now, but I think that 14px base size will be an improvement.

myasonik · 2020-02-19T19:33:19Z

Just wanted to check if you ran all these through a contrast check? (We want to hit a 5.4:1 ratio between the text and the backgrounds.)

snide · 2020-02-19T19:42:51Z

@myasonik Think you mean 4.5. We have plenty of tooling around accessibility checks for colors in the docs. You can safely assume @cchaos or I will watch over these PRs as they come in 😄

daveyholler · 2020-02-19T20:13:04Z

@johnbarrierwilson

Is that screenshot using the current 16px base font size? Will it shrink a little when we switch to 14px? Just curious. It doesn't look bad right now, but I think that 14px base size will be an improvement.

This is 16px atm. PR for font size changes will come later.

cchaos · 2020-02-19T22:36:12Z

This one is going to be a bit trickier because we would like avoid as much direct copy/pastes and duplicate styles as possible. It just makes maintenance that much harder. I will pull down this branch and see what kind of solutions I can come up with. It'll probably be a mix of making more mixins and/or breaking apart styles based on purpose. So give me a bit to fiddle with this one.

…hem.

- Only showing the Amsterdam callout when they’ve switched to the Amsterdam theme - Added `makeGraphicColor()` SASS color function - Added defaults to `makeHighContrastColor()` SASS color function

…ght theme only

- Broke `euiButton()` mixin down to smaller mixins for easier reuse of base styles - Moved `overrides/` folder out of `global_styling` and importing __after__ component imports - Only overriding what __needs__ to/what is __different__

kibanamachine · 2020-02-25T01:44:51Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

kibanamachine · 2020-02-25T23:11:25Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

kibanamachine · 2020-02-26T18:01:49Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

kibanamachine · 2020-02-27T18:51:46Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

kibanamachine · 2020-02-27T22:30:02Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

cchaos · 2020-02-27T23:11:39Z

🎉 This one is ready for final review. Please read through the summary first as it should address a few things that might come up.

cchaos

I did a final code review, and it all LGTM 👍

kibanamachine · 2020-02-27T23:32:42Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

snide

Code looks good, buttons look good. Nice work.

I see a break on context menus in the new style where the popover can't calculate a best position. I haven't dug too far into the reason yet, it doesn't look related to this PR. Only happens in the new theme. Does not seem related to this PR because I'm noticing it on the compiled site. Super weird and wondering if it has to do with the dynamic css switch.

Not for this PR, but my guess is we still have some work to do on Focus states for buttons. We do it a little differently (sometimes darken, sometimes lighten, sometimes outline...etc).

The filled, but disabled Ghost buttons seem like they might need a separate check for their text color. Feels like it pops too much for a disabled button.

cchaos · 2020-02-28T15:11:22Z

I see a break on context menus in the new style where the popover can't calculate a best position. I haven't dug too far into the reason yet, it doesn't look related to this PR. Only happens in the new theme. Does not seem related to this PR because I'm noticing it on the compiled site. Super weird and wondering if it has to do with the dynamic css switch.

OMG, thank you for pointing out that it's only in this theme. I kept seeing this error too, but couldn't find a pattern to it. And every time I checked on published docs it was fine (because I never switched from the default theme).

I'll see if @chandlerprall can take a look outside of this PR

The filled, but disabled Ghost buttons seem like they might need a separate check for their text color. Feels like it pops too much for a disabled button.

I'm now setting the text color to be 2.0 contrast from background color of filled ghost buttons

cchaos · 2020-02-28T15:25:02Z

@chandlerprall Scratch that request, I've found the root of the problem stemming from an early change the the EuiPanel mixin. I'll be able to push the fix up to this PR.

kibanamachine · 2020-02-28T15:29:19Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

kibanamachine · 2020-02-28T15:55:21Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

snide

Awesome. Thanks for the quick fixes. That disabled ghost button looks much better now.

kibanamachine · 2020-02-28T17:30:21Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

This reverts commit 8575def.

kibanamachine · 2020-02-28T22:17:55Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

chandlerprall

Changes LGTM; looked to see if TypeScript could be helpful around that deprecation but didn't find anything

kibanamachine · 2020-03-04T19:31:19Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2874/

daveyholler requested review from ryankeairns, cchaos and johnbarrierwilson February 19, 2020 18:50

cchaos added Amsterdam labels Feb 19, 2020

daveyholler and others added 11 commits February 21, 2020 16:15

Saturating colors and providing contrast mixin for text.

39f850e

Adding text-specific color variables and updating components to use t…

3752715

…hem.

Adding a danger text color as well.

c500652

Update some more downstream components

a5b5ec7

- Only showing the Amsterdam callout when they’ve switched to the Amsterdam theme - Added `makeGraphicColor()` SASS color function - Added defaults to `makeHighContrastColor()` SASS color function

Added toggle to color guidelines to show text variant in amsterdam-li…

e91899e

…ght theme only

Updating button styles.

31a0dcf

Updating button styles.

1207429

Figuring out a better way to override

f31068a

- Broke `euiButton()` mixin down to smaller mixins for easier reuse of base styles - Moved `overrides/` folder out of `global_styling` and importing __after__ component imports - Only overriding what __needs__ to/what is __different__

Fixing the ghost colored disabled buttons

e272091

Woking on getting the colors and states right.

57665dc

Removing rogue mixin import.

d976768

daveyholler force-pushed the am/updating-buttons branch from c4aadaf to d976768 Compare February 25, 2020 01:26

daveyholler added 2 commits February 25, 2020 14:42

Merge remote-tracking branch 'upstream/master' into am/updating-buttons

a470496

Merge conflict in colors.js.

eb1601c

daveyholler mentioned this pull request Feb 26, 2020

[Amsterdam] Updating base text sizing #2936

Merged

5 tasks

CL

f16ef46

snide mentioned this pull request Feb 26, 2020

[Meta] Deprecation schedule #1469

Open

35 tasks

cl

f27a937

addressing button states in dark mode.

bafd0f7

Changing transparency instead of shade

74cdbfa

cchaos requested review from chandlerprall and snide February 27, 2020 23:11

cchaos approved these changes Feb 27, 2020

View reviewed changes

snide reviewed Feb 28, 2020

View reviewed changes

Fix contrast of filled ghost disabled button

dcaab4c

cchaos requested a review from snide February 28, 2020 15:11

Temporarily fix context menu

8575def

snide approved these changes Feb 28, 2020

View reviewed changes

Merge branch 'master' into am/updating-buttons

a260cb3

chandlerprall mentioned this pull request Feb 28, 2020

Better guarantee that a popover position can be found #2948

Merged

2 tasks

Revert "Temporarily fix context menu"

54a5eb2

This reverts commit 8575def.

chandlerprall approved these changes Mar 4, 2020

View reviewed changes

Merge branch 'master' into am/updating-buttons

6656f3a

daveyholler merged commit 8c3ca5f into elastic:master Mar 4, 2020

chandlerprall mentioned this pull request Mar 16, 2020

Upgrade EUI to 21.0.1 elastic/kibana#60282

Merged

cchaos mentioned this pull request Sep 23, 2020

Finish Dec-2019 & July-2020 deprecations #4065

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Amsterdam] Updating button styles #2874

[Amsterdam] Updating button styles #2874

daveyholler commented Feb 19, 2020 •

edited by cchaos

Loading

johnbarrierwilson commented Feb 19, 2020

myasonik commented Feb 19, 2020

snide commented Feb 19, 2020

daveyholler commented Feb 19, 2020

cchaos commented Feb 19, 2020

kibanamachine commented Feb 25, 2020

kibanamachine commented Feb 25, 2020

kibanamachine commented Feb 26, 2020

kibanamachine commented Feb 27, 2020

kibanamachine commented Feb 27, 2020

cchaos commented Feb 27, 2020

cchaos left a comment

kibanamachine commented Feb 27, 2020

snide left a comment

cchaos commented Feb 28, 2020

cchaos commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

snide left a comment

kibanamachine commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

chandlerprall left a comment

kibanamachine commented Mar 4, 2020

[Amsterdam] Updating button styles #2874

[Amsterdam] Updating button styles #2874

Conversation

daveyholler commented Feb 19, 2020 • edited by cchaos Loading

Summary

Details

Consumers

Follow Ups

Dev

Disabled styles

Ghost buttons

Deprecations #1469

text color EuiButton (and alts)

disabled color EuiButton (and alts)

Checklist

johnbarrierwilson commented Feb 19, 2020

myasonik commented Feb 19, 2020

snide commented Feb 19, 2020

daveyholler commented Feb 19, 2020

cchaos commented Feb 19, 2020

kibanamachine commented Feb 25, 2020

kibanamachine commented Feb 25, 2020

kibanamachine commented Feb 26, 2020

kibanamachine commented Feb 27, 2020

kibanamachine commented Feb 27, 2020

cchaos commented Feb 27, 2020

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 27, 2020

snide left a comment

Choose a reason for hiding this comment

cchaos commented Feb 28, 2020

cchaos commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

snide left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 28, 2020

kibanamachine commented Feb 28, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 4, 2020

daveyholler commented Feb 19, 2020 •

edited by cchaos

Loading

`text` color EuiButton (and alts)

`disabled` color EuiButton (and alts)