[EuiButtonGroup] Convert to Emotion #6841

cee-chen · 2023-06-14T03:27:49Z

Summary

This PR converts EuiButtonGroup and EuiButtonGroupButton to Emotion. I recommend following along by commit. TL;DR of changes:

Adds a new internal euiOutline() CSS-in-JS util (a lighter weight version of euiFocusRing(), without the focus-visible styles)
Removes the component's usage of EuiButtonDisplayDeprecated in favor of the new internal EuiButtonDisplay component, and then removes EuiButtonDisplayDeprecated entirely
Fixes text truncation not working correctly on mobile/smaller screen widths (free fix w/ new EuiButtonDisplay usage)
Fixes border separation between buttons not responding to light/dark mode
Fixes focus ring not working as expected
- Outlines now correctly only appear on keyboard tab and not on mouse click
- Outline colors on compressed button groups now correctly use the same color as the button fill
Attempts to partially address [EuiButtonGroup] Better differentiate the current selection #6819 by increasing the font weight of selected options
Cleans up a lot of unnecessary CSS cruft
✨ Adds a new Storybook page for EuiButtonGroup, making permutations significantly easier to test

QA

Compare staging to production
Confirm the components look approximately the same as before without visual regression
Note that the visuals are not exactly the same - Emotion changes some slight rendering around font size and line height (now inherits directly from EuiButtonDisplay. I consider this to be an intentional change to match other existing buttons
[OPTIONAL] View component in Storybook
- gh pr checkout 6841
- yarn storybook
- Go to http://localhost:6006/?path=/story/euibuttongroup--playground and test multiple permutations of sizes, colors, etc

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress tests
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately
~~- [ ] Checked for breaking changes and labeled appropriately~~ - Even though EuiButtonDisplayDeprecated was deleted, this was not a publicly exported component and therefore is not a breaking change

- [ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~

Emotion checklist

Acceptance criteria

All SCSS files have been removed from the component(s) directory
~~- [ ] All SCSS overrides have been removed from the Amsterdam theme~~ - No overrides for this component
~~- [ ] Any dependent components are identified in a new issue~~

Checklists

Kibana usage

Search Kibana's codebase for {euiComponent}- (case sensitive) to check for usage of modifier classes
~~- [ ] If usage exists, consider converting to a data attribute so that consumers still have something to hook into~~ - Retained .euiButtonGroupButton-isSelected className, which is the primary modifier usage

General

Output CSS matches the previous CSS (works as expected in all browsers)
Rendered className(s) read as expected in snapshots and browsers
Checked component playground

Unit tests

shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
Converted from Enzyme to RTL
~~- [ ] Removed any mounted snapshots~~

Sass/Emotion conversion process

Removed component from src/components/index.scss

~~- [ ] Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)~~
~~- [ ] Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)~~
~~- [ ] Removed or converted component-specific Sass vars/mixins to exported JS versions~~
~~- [ ] Listed var/mixin removals in changelog~~
~~- [ ] Ran yarn compile-scss to update var/mixin JSON files~~
~~- [ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)~~
~~- [ ] Added an @warn deprecation message within the global_styling/mixins/{component}.scss file~~

CSS tech debt

Wrapped all animations or transitions in euiCanAnimate
Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)
~~- [ ] Used gap property to add margin between items if using flex~~

DOM Cleanup

Did NOT remove any block/element classNames (e.g. euiComponent, euiComponent__child)
SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.

Kibana due diligence

Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding **/target, **/*.snap, **/*.storyshot for less noise) for eui{Component} (case sensitive) to find:
~~- [ ] Any test/query selectors that will need to be updated~~ None
~~- [ ] Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted~~ None
~~- [ ] Any direct className usages that will need to be refactored (e.g. someone calling the euiBadge class on a div instead of simply using the EuiBadge component)~~ None

Extras/nice-to-have

Reduced specificity where possible (usually by reducing nesting and class name chaining)
~~- [ ] Documentation pass~~
~~- [ ] Check for issues in the backlog that could be a quick fix for that component~~
~~- [ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about~~

- required for deprecation of old component, used by `EuiButtonGroup` + rename `element` usage in `EuiButtonGroupButton` to match existing prefix syntax

kibanamachine · 2023-06-14T03:52:18Z

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

kibanamachine · 2023-06-14T04:34:17Z

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

kibanamachine · 2023-06-14T05:11:11Z

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

kibanamachine · 2023-06-14T05:55:39Z

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

- `label`s that wrap `input`s don't need a `for` attribute, and buttons really aren't benefiting from randomly generated IDs/data-test-subjs

- update focus utils with a new `euiOutline` helper that only exports the outline CSS and not the `:focus-visible` - Button group focus-visible behavior wasn't working as expected - `:has(:focus-visible)` is what's needed for `:focus-within` keyboard behavior, but it needs a Firefox interim workaround

…nternal - New `EuiButtonDisplay` component makes a lot of the old Sass styles unnecessary + add euiCanAnimate

- Disabled & selected buttons need a custom style + make all selected buttons have a slightly heavier font weight + remove unused(?) z-index CSS

- handles `border-radius` - handles faux box-shadow borders (with better cleanup) - compressed vs uncompressed styles

- remove unnecessary `box-shadow` affordances - doesn't appear to be a thing anymore in Amsterdam theme - remove `overflow hidden` - doesn't seem to be needed - remove `groupSizeToClassNameMap`

- compressed font weight is not the same as default button weight - [opinionated] increase uncompressed font weight to make it more obvious which is chosen

There's no longer any components using it in EUI now that `EuiButtonGroup` has been converted to Emotion

- Convert Enzyme to RTL - Reorganize tests slightly - Add tests for ensuring `css` properly merges

kibanamachine · 2023-06-14T07:24:09Z

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

breehall

This looks great Cee! I compared staging to prod and noted the changes mentioned in your description. There are so many QOL changes here! The emboldened text on the selected buttons makes a difference.

I also spun up Storybook and what an experience! This made it so easy to test different permutations of EuiButtonGroup and it's so much easier to use than the Playground.

❓ Question

Is it expected that the keyboard controls use the left and right arrows for single selection button groups, but the tab button for multi-select button groups?

👀 Observation for Future

The button backgrounds in dark mode have a fairly low contrast ratio to the page background. This is something we may want to address at a later time.

breehall · 2023-06-14T19:29:49Z

src/components/button/button_group/button_group.test.tsx

-  describe('type', () => {
-    test('single is rendered', () => {
-      const component = render(
+  describe('type="single"', () => {


I like how you broke these up and renamed them. They're easier to read and follow

I wanted to clean up the other props/unit tests in a similar vein, but I figure they're better candidates for saving for storybook visual snapshots or similar tests in the future!

cee-chen · 2023-06-14T20:18:23Z

❓ Question
Is it expected that the keyboard controls use the left and right arrows for single selection button groups, but the tab button for multi-select button groups?

Yes, the single selections are using <input type="radio">s under the hood which behave exactly as you mention (only 1 radio option is tabbable, use the arrow keys to navigate within options).

TBH, I'm not sure if I would have chosen that approach myself necessarily as I agree it feels odd compared to multiselect, but EuiButtonGroup was already set up this way to start, and I don't think it needs an overhaul at this point.

👀 Observation for Future
The button backgrounds in dark mode have a fairly low contrast ratio to the page background. This is something we may want to address at a later time.

Good eye, agreed! Honestly I really like the compressed styling compared to the non-compressed, as it resolves a lot of the aforementioned contrast issues thanks to its border(s). If we were to try and make a big set of visual changes to this component, I'd be really tempted to make that the default style.

cee-chen · 2023-06-14T20:19:07Z

Thanks a million for the speedy review Bree!

kibanamachine · 2023-06-20T16:00:40Z

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

`eui@82.1.0` ⏩ `83.0.0` ⚠️ The biggest change in this PR by far is the `EuiButtonEmpty` Emotion conversion, which changes the DOM structure of the button slightly as well as several CSS classes around it. EUI has attempted to convert any custom EuiButtonEmpty CSS overrides where possible, but would super appreciate it if CODEOWNERS checked their touched files. If anything other than a snapshot or test was touched, please double check the display of your button(s) and confirm everything still looks shipshape. Feel free to ping us for advice if not. --- ## [`83.0.0`](https://github.com/elastic/eui/tree/v83.0.0) **Bug fixes** - Fixed `EuiPaginationButton` styling affected by `EuiButtonEmpty`'s Emotion conversion ([#6893](elastic/eui#6893)) **Breaking changes** - Removed `isPlaceholder` prop from `EuiPaginationButton` ([#6893](elastic/eui#6893)) ## [`82.2.1`](https://github.com/elastic/eui/tree/v82.2.1) - Updated supported Node engine versions to allow Node 16, 18 and >=20 ([#6884](elastic/eui#6884)) ## [`82.2.0`](https://github.com/elastic/eui/tree/v82.2.0) - Updated EUI's SVG icons library to use latest SVGO v3 optimization ([#6843](elastic/eui#6843)) - Added success color `EuiNotificationBadge` ([#6864](elastic/eui#6864)) - Added `badgeColor` prop to `EuiFilterButton` ([#6864](elastic/eui#6864)) - Updated `EuiBadge` to use CSS-in-JS for named colors instead of inline styles. Custom colors will still use inline styles. ([#6864](elastic/eui#6864)) **CSS-in-JS conversions** - Converted `EuiButtonGroup` and `EuiButtonGroupButton` to Emotion ([#6841](elastic/eui#6841)) - Converted `EuiButtonIcon` to Emotion ([#6844](elastic/eui#6844)) - Converted `EuiButtonEmpty` to Emotion ([#6863](elastic/eui#6863)) - Converted `EuiCollapsibleNav` and `EuiCollapsibleNavGroup` to Emotion ([#6865](elastic/eui#6865)) - Removed Sass variables `$euiCollapsibleNavGroupLightBackgroundColor`, `$euiCollapsibleNavGroupDarkBackgroundColor`, and `$euiCollapsibleNavGroupDarkHighContrastColor` ([#6865](elastic/eui#6865)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Jeramy Soucy <jeramy.soucy@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Add support for element prop to EuiButtonDisplay

7e5f100

- required for deprecation of old component, used by `EuiButtonGroup` + rename `element` usage in `EuiButtonGroupButton` to match existing prefix syntax

cee-chen force-pushed the emotion/button-group branch 3 times, most recently from 25bb55a to 16a8487 Compare June 14, 2023 04:46

cee-chen added the emotion label Jun 14, 2023

cee-chen force-pushed the emotion/button-group branch 2 times, most recently from d500bff to a3832be Compare June 14, 2023 05:31

cee-chen requested a review from breehall June 14, 2023 05:43

cee-chen marked this pull request as ready for review June 14, 2023 05:44

cee-chen force-pushed the emotion/button-group branch from a3832be to 6fefe91 Compare June 14, 2023 06:53

cee-chen added 13 commits June 13, 2023 23:55

Remove unnecessary generated id from EuiButtonGroup

2c6f8c4

- `label`s that wrap `input`s don't need a `for` attribute, and buttons really aren't benefiting from randomly generated IDs/data-test-subjs

[EuiButtonGroupButton] Convert to non-deprecated EuiButtonDisplay i…

fed65a4

…nternal - New `EuiButtonDisplay` component makes a lot of the old Sass styles unnecessary + add euiCanAnimate

[EuiButtonGroupButton] Convert state styles

d8b6c5f

- Disabled & selected buttons need a custom style + make all selected buttons have a slightly heavier font weight + remove unused(?) z-index CSS

[EuiButtonGroupButton] Convert child content & text CSS

4da3346

[EuiButtonGroupButton] Convert size-specific styles

45f48a0

- handles `border-radius` - handles faux box-shadow borders (with better cleanup) - compressed vs uncompressed styles

[EuiButtonGroup] Convert styles to Emotion

04a4a65

- remove unnecessary `box-shadow` affordances - doesn't appear to be a thing anymore in Amsterdam theme - remove `overflow hidden` - doesn't seem to be needed - remove `groupSizeToClassNameMap`

Fix iconOnly padding; fix font weights

f1c144e

- compressed font weight is not the same as default button weight - [opinionated] increase uncompressed font weight to make it more obvious which is chosen

Delete button group Sass files

f3b4a1d

Delete EuiButtonDisplayDeprecated

82dd6b7

There's no longer any components using it in EUI now that `EuiButtonGroup` has been converted to Emotion

Update tests & snapshots

9020afd

- Convert Enzyme to RTL - Reorganize tests slightly - Add tests for ensuring `css` properly merges

Add Storybook playground

2884fd0

changelog

52625bd

cee-chen force-pushed the emotion/button-group branch from 6fefe91 to 52625bd Compare June 14, 2023 06:57

breehall approved these changes Jun 14, 2023

View reviewed changes

cee-chen merged commit c19625a into elastic:main Jun 14, 2023

cee-chen deleted the emotion/button-group branch June 14, 2023 20:19

This was referenced Jun 22, 2023

[Emotion] EuiButton #6388

Closed

[EuiButtonGroup] Better differentiate the current selection #6819

Closed

breehall mentioned this pull request Jun 28, 2023

Upgrade EUI to 83.0.0 elastic/kibana#160813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiButtonGroup] Convert to Emotion #6841

[EuiButtonGroup] Convert to Emotion #6841

cee-chen commented Jun 14, 2023 •

edited

Loading

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

breehall left a comment

breehall Jun 14, 2023

cee-chen Jun 14, 2023 •

edited

Loading

cee-chen commented Jun 14, 2023 •

edited

Loading

cee-chen commented Jun 14, 2023

kibanamachine commented Jun 20, 2023

[EuiButtonGroup] Convert to Emotion #6841

[EuiButtonGroup] Convert to Emotion #6841

Conversation

cee-chen commented Jun 14, 2023 • edited Loading

Summary

QA

General checklist

Emotion checklist

Acceptance criteria

Checklists

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

kibanamachine commented Jun 14, 2023

breehall left a comment

Choose a reason for hiding this comment

breehall Jun 14, 2023

Choose a reason for hiding this comment

cee-chen Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen commented Jun 14, 2023 • edited Loading

cee-chen commented Jun 14, 2023

kibanamachine commented Jun 20, 2023

cee-chen commented Jun 14, 2023 •

edited

Loading

cee-chen Jun 14, 2023 •

edited

Loading

cee-chen commented Jun 14, 2023 •

edited

Loading