[EuiButtonIcon] Add display prop #4466

andreadelrio · 2021-01-31T17:05:17Z

Summary

Added display prop to support two more styles. Options now are: base (similar to default EuiButton), empty and fill (similar to EuiButton with fill).
Added size prop to also match that of the other button types

Also added a section about how this enable "split" button behaviors.

Note:

There's an issue happening in Chrome on hover. It seems like the button briefly gains width. Haven't been able to track down more details but seems like it occurs when a button has one element (an svg in this case). Checked other browsers and they behave ok. See:

UPDATE: This is def a chrome bug and not specific to this PR. Will deal with later.

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added documentation
~~- [ ] Checked Code Sandbox works for the any docs examples~~
Added or updated jest tests
~- [ ] Checked for breaking changes and labeled appropriately
~~- [x] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

kibanamachine · 2021-01-31T17:37:48Z

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

kibanamachine · 2021-01-31T18:06:02Z

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

andreadelrio · 2021-02-01T15:45:40Z

cchaos

This is great! I'm excited to have more prominent icon-only buttons. One of the main reasons to do this as well is to allow for the idea of split buttons by just using two buttons side-by-side, one that is icon-only but looks the same as our other ones. Like:

So I'd like to be able to take this a step further and also align the size of the button to match the sizes of our other buttons. For instance, we already weirdly have a size prop that doesn't do anything. 🤷

So let's use that and expand it to include the sizes xs | s | m. The xs size would be the default as it is now 24x24px and would match the EuiButtonEmpty.size = 'xs'.

Then size s would start to match the sizing of our normal EuiButton.size="s".

What I did notice, however, is that the new styles (fill and default) actually increase the overall size of the button to 26x26px because of the border. We would need to accomodate for that border and ensure it still renders at the same overall square size as the empty version.

chandlerprall · 2021-02-03T19:05:19Z

This is great, thanks @andreadelrio! Would be nice to find a better name than default for the existing style, or allow an undefined on the prop to not "apply" a style.

cchaos · 2021-02-03T19:23:12Z

Oooh I like the undefined idea!

kibanamachine · 2021-02-05T01:35:32Z

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

cchaos

Thanks for making those updates! You can probably update the CL entry too now to include the size prop.

Quick pass on the visual design and it all looks right except for the shadows. There's some discrepancies between how the normal buttons behave and the icon buttons.

In the default theme, the fill style has no shadow at all, but it should. Also, the disabled buttons are moving when hovered, but they shouldn't.

In the Amsterdam theme, the undefined version has a shadow, but shouldn't. We've removed all the shadows from buttons in the Amsterdam theme.

src/components/button/button_icon/button_icon.tsx

src-docs/src/views/button/button_icon.js

src-docs/src/views/button/button_example.js

kibanamachine · 2021-02-23T21:08:32Z

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

Basically copy/pasted all .euiButton styles. Created new `$euiButtonHeightXSmall` sass var

kibanamachine · 2021-02-23T23:08:37Z

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

cchaos

I took this one over so we can have these updates for Amsterdam. Most of the changes are actually snapshots from added classes.

cchaos · 2021-02-23T23:37:28Z

src/components/button/button.tsx

-  /**
-   * Set for deprecation 2/26/20
-   * This color button can easily be confused with disabled, it should not be used
-   */
  | 'text';


I removed this deprecation notice because there are actually cases where the text option is viable

cchaos · 2021-02-23T23:38:32Z

src/components/button/button_icon/_button_icon.scss

+// This file has lots of modifiers and is somewhat nesty by nature
+// sass-lint:disable nesting-depth


In this file, I basically copy/pasted directly from the .euiButton styles. I didn't want to accidentally introduce any bugs into the .euiButton styles by trying to make more mixins or such, so that's why I just did a manual copy/paste.

cchaos · 2021-02-23T23:38:55Z

src/components/button/button_icon/_variables.scss

@@ -2,11 +2,10 @@
 $euiButtonIconTypes: (
  accent: $euiColorAccentText,
  danger: $euiColorDangerText,
-  disabled: $euiButtonColorDisabledText,


This wasn't being used in this manner

cchaos · 2021-02-23T23:39:32Z

src/components/button/button_icon/_variables.scss

-  text: $euiTextColor,
+  text: $euiColorDarkShade,


The text color of the empty version is manually changed to $euiTextColor in the loop, so this change only affects the fill and base versions.

cchaos · 2021-02-23T23:40:35Z

src/components/button/button_icon/button_icon.tsx

+  /**
+   * Set for deprecation 2/23/21
+   * This color button is close enough to text to be duplicative
+   */
  | 'subdued'


This color is too close to both disabled and the text versions of the button so I think we should deprecate it in favor of text.

Out of date

kibanamachine · 2021-02-24T17:36:01Z

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

thompsongl

Code changes LGTM!

src/components/button/button_icon/button_icon.tsx

Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>

kibanamachine · 2021-03-02T16:00:12Z

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

elizabetdev

Tested in Chrome, Firefox, Edge, and Safari. LGTM! 🎉

I just have two questions/suggestions.

Should the disabled empty EuiButtonIcon have a hover state? It seems weird to me.

Screen.Recording.2021-03-02.at.04.24.PM.mov

For consistency when we're talking about requiring an aria-label:

Should we use a EuiCallOut instead? Like we're using for other a11y suggestions.

cchaos · 2021-03-02T16:56:00Z

Bah! I can't believe I missed that hover state. Good catch! Sure I can also update that doc message to be in a callout.

kibanamachine · 2021-03-02T18:34:52Z

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

andreadelrio added 6 commits January 25, 2021 18:42

add display prop to EuiButtonIcon

d65a911

cleanup

d1c5ae2

cleanup example

e37c499

idk whats happening anymore :)

ae7f1b8

more things

aa03f18

update dg snap

1cc903a

cl

80d32af

andreadelrio requested a review from cchaos January 31, 2021 18:52

cchaos reviewed Feb 2, 2021

View reviewed changes

andreadelrio added 3 commits February 4, 2021 09:25

update display prop and add sizes

2cb7bde

cl

9307e97

adjust control_bar

7a38855

andreadelrio requested a review from cchaos February 5, 2021 14:06

cchaos previously requested changes Feb 5, 2021

View reviewed changes

src/components/button/button_icon/button_icon.tsx Outdated Show resolved Hide resolved

src-docs/src/views/button/button_icon.js Outdated Show resolved Hide resolved

src-docs/src/views/button/button_example.js Outdated Show resolved Hide resolved

Merge branch 'master' into buttonIcon-styles

969aa02

cchaos added 4 commits February 23, 2021 16:52

Default theme updates

ebf3dd2

Basically copy/pasted all .euiButton styles. Created new `$euiButtonHeightXSmall` sass var

Snaps

f8630a4

Fixing up Amsterdam styles

88daff3

Fix text color, clean up props

ad63e5c

cchaos self-requested a review February 23, 2021 22:44

cchaos added 2 commits February 23, 2021 18:15

Changing undefined being the standard button type to base

3e03142

Updating docs

c49de4e

cchaos added 2 commits February 23, 2021 18:32

Merge remote-tracking branch '2_upstream/master' into buttonIcon-styles

6df79c2

cl

c6d4625

cchaos requested review from thompsongl and elizabetdev and removed request for cchaos February 23, 2021 23:33

cchaos reviewed Feb 23, 2021

View reviewed changes

Fixing some component usages of EuiButtonIcon

29524d8

thompsongl approved these changes Mar 2, 2021

View reviewed changes

src/components/button/button_icon/button_icon.tsx Outdated Show resolved Hide resolved

cchaos and others added 2 commits March 2, 2021 10:36

Update src/components/button/button_icon/button_icon.tsx

76da9d5

Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>

Merge branch 'master' into buttonIcon-styles

d78fe6e

elizabetdev approved these changes Mar 2, 2021

View reviewed changes

cchaos and others added 2 commits March 2, 2021 13:06

Fixed disabled hover style in Amsterdam and added a11y callout

ac29ed6

Merge branch 'master' into buttonIcon-styles

d1b9e22

cchaos merged commit 6c005f4 into elastic:master Mar 2, 2021

chandlerprall mentioned this pull request Mar 5, 2021

Upgrade EUI to v31.10.0 elastic/kibana#93796

Merged

dependabot bot mentioned this pull request Mar 15, 2021

yarn upgrade: bump @elastic/eui from 31.8.0 to 31.10.0 Noxsios/config-composer#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiButtonIcon] Add display prop #4466

[EuiButtonIcon] Add display prop #4466

andreadelrio commented Jan 31, 2021 •

edited by cchaos

Loading

kibanamachine commented Jan 31, 2021

kibanamachine commented Jan 31, 2021

andreadelrio commented Feb 1, 2021 •

edited

Loading

cchaos left a comment

chandlerprall commented Feb 3, 2021

cchaos commented Feb 3, 2021

kibanamachine commented Feb 5, 2021

cchaos left a comment

kibanamachine commented Feb 23, 2021

kibanamachine commented Feb 23, 2021

cchaos left a comment

cchaos Feb 23, 2021

cchaos Feb 23, 2021

cchaos Feb 23, 2021

cchaos Feb 23, 2021

cchaos Feb 23, 2021

kibanamachine commented Feb 24, 2021

thompsongl left a comment

kibanamachine commented Mar 2, 2021

elizabetdev left a comment

cchaos commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

		// This file has lots of modifiers and is somewhat nesty by nature
		// sass-lint:disable nesting-depth

[EuiButtonIcon] Add display prop #4466

[EuiButtonIcon] Add display prop #4466

Conversation

andreadelrio commented Jan 31, 2021 • edited by cchaos Loading

Summary

Checklist

kibanamachine commented Jan 31, 2021

kibanamachine commented Jan 31, 2021

andreadelrio commented Feb 1, 2021 • edited Loading

cchaos left a comment

Choose a reason for hiding this comment

chandlerprall commented Feb 3, 2021

cchaos commented Feb 3, 2021

kibanamachine commented Feb 5, 2021

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 23, 2021

kibanamachine commented Feb 23, 2021

cchaos left a comment

Choose a reason for hiding this comment

cchaos Feb 23, 2021

Choose a reason for hiding this comment

cchaos Feb 23, 2021

Choose a reason for hiding this comment

cchaos Feb 23, 2021

Choose a reason for hiding this comment

cchaos Feb 23, 2021

Choose a reason for hiding this comment

cchaos Feb 23, 2021

Choose a reason for hiding this comment

kibanamachine commented Feb 24, 2021

thompsongl left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 2, 2021

elizabetdev left a comment

Choose a reason for hiding this comment

cchaos commented Mar 2, 2021

kibanamachine commented Mar 2, 2021

andreadelrio commented Jan 31, 2021 •

edited by cchaos

Loading

andreadelrio commented Feb 1, 2021 •

edited

Loading