[Emotion] Convert EuiButtonIcon #6844

cee-chen · 2023-06-15T06:55:07Z

Summary

This PR (hopefully) should be fairly quick to review/verify. Not a lot of major changes going on here (other than a million snapshots).

Removes internal useEuiButtonRadiusCSS hook in favor of a more agnostic euiButtonSizeMap - using the raw size data is less strict & generates fewer classNames
Converts EuiButtonIcon to Emotion, removing/cleaning up unnecessary CSS
Removes .euiButtonIcon--[size] modifier classes
Fixes isLoading button icon spinners to have the same background color contrast as other button loading spinners

QA

Compare production against staging and confirm that EuiButtonIcon's demo example looks the same between the two
gh pr checkout 6844 && yarn storybook
Go to the EuiButtonIcon playground and test multiple color/size permutations and confirm they look the same as before

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
A changelog entry exists and is marked appropriately

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~~ - All dependent usages in EUI are calling the generic .euiButtonIcon

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~~ - no major Kibana usages

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

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~~
~~- [ ] Wrapped all animations or transitions in euiCanAnimate~~

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 Sass or CSS that will need to be updated, particularly if a component Sass var was deleted~~ None
Any test/query selectors that will need to be updated
- 2 minor usages as test queries that will need to be updated
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)
- 1 usage that will need to be fixed

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~~

- Remove `useEuiButtonRadiusCSS` in favor of a more agnostic `euiButtonSizeMap` - using the raw data is less strict and generates fewer classNames - `EuiButtonIcon` will shortly need the height/radius map and does not need to use all of `EuiButtonDisplay`'s logic (e.g. doesn't need fullWidth, or a content/text wrapper) - Move `_buttonSize` to internal fn to make it unexportable / clearer that its CSS is specific to `EuiButtonDisplay`

… to separate file

+ focus CSS comes from a hook

- this can mostly be simplified down from its Sass mixin to just the cursor CSS - the loading spinner color comment + logic is copied from `EuiButtonDisplay` - it previously existed in the `euiButtonContentDisabled` Sass mixin, but was targeting a selector that did not exist in EuiButtonIcon

+ convert enum types to new preferred syntax + move type enums to top of file

+ configure all playgrounds to sort props by required first, then alphabetical

kibanamachine · 2023-06-20T15:41:38Z

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

kibanamachine · 2023-06-20T15:55:50Z

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

kibanamachine · 2023-06-20T16:25:30Z

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

- preload all icons into the cache at once in the preview - this increases Storybook's startup time somewhat significantly, but will probably be necessary until we resolve dynamic icons for SSR/Vite

kibanamachine · 2023-06-21T04:22:06Z

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

breehall

Great work with this Cee! The new DRY utils are nice and it's great to see patterns in EuiButtonIcon being updated to match our other components.
I pulled locally to test and also compared staging against main and didn't find any visual changes. Storybook looks good as well.

❓ Question:
Is the isSelected toggle supposed to change the state in our docs and Storybook? The behavior is the same in both places, so it's not a blocker. I just figured I would ask for my own knowledge.

When buttons are filled or base, there's no visual diff in whether the button is selected or not. For empty, the button fills with the color, but I don't see that happen when I toggle isSelected in the Playground or in Storybook.

EA36E09C-6A64-43CB-A086-B84292D69B9D_2_0_a.mov

breehall · 2023-06-21T17:30:37Z

src/components/button/button_display/_button_display.styles.ts

+  const sizes = euiButtonSizeMap(euiThemeContext);
+
+  const _buttonSize = (sizeKey: EuiButtonDisplaySizes) => {
+    const size = sizes[sizeKey];
+    return css`
+      ${logicalCSS('height', size.height)}
+      line-height: ${size.height}; /* Prevents descenders from getting cut off */
+      ${euiFontSize(euiThemeContext, size.fontScale)}
+      border-radius: ${size.radius};
+    `;
+  };


🧼 Super clean!

breehall · 2023-06-21T17:35:13Z

src/components/button/button_icon/button_icon.tsx

+    // When the button is disabled the text gets gray
+    // and in some buttons the background gets a light gray
+    // for better contrast we want to change the border of the spinner
+    // to have the same color of the text. This way we ensure the borders
+    // are always visible. The default spinner color could be very light.
+    const loadingSpinnerColor = isDisabled
+      ? { border: 'currentcolor' }
+      : undefined;
+
+    buttonIcon = (
+      <EuiLoadingSpinner size={loadingSize} color={loadingSpinnerColor} />
+    );


Nice accessibility consideration!

breehall · 2023-06-21T17:39:55Z

src/components/button/button_icon/button_icon.tsx

+export const SIZES = ['xs', 's', 'm'] as const;
+export type EuiButtonIconSizes = (typeof SIZES)[number];

-export const DISPLAYS = keysOf(displayToClassNameMap);
-type EuiButtonIconDisplay = keyof typeof displayToClassNameMap;
+export const DISPLAYS = ['base', 'empty', 'fill'] as const;
+type EuiButtonIconDisplay = (typeof DISPLAYS)[number];


Happy to see this being updated to the pattern we use in most other EUI components.

cee-chen · 2023-06-21T18:37:29Z

❓ Question:
Is the isSelected toggle supposed to change the state in our docs and Storybook? The behavior is the same in both places, so it's not a blocker. I just figured I would ask for my own knowledge.

Great question! The isSelected prop for buttons is primarily for accessibility - it simply adds an aria-pressed attribute that informs screen readers that the button represents a selection state of some sort. There isn't a visual reflection of this state.

cee-chen · 2023-06-21T18:39:53Z

Thanks a million for the speedy review Bree!

`[email protected]` ⏩ `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 <[email protected]> Co-authored-by: Jeramy Soucy <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

cee-chen force-pushed the emotion/button-icon branch 2 times, most recently from abaad0b to a691211 Compare June 20, 2023 15:11

cee-chen added 9 commits June 20, 2023 08:32

[EuiButtonIcon] Implement sizes styles + move existing Emotion styles…

2f9db9a

… to separate file

[EuiButtonIcon] Convert remaining base button style

8558e85

+ focus CSS comes from a hook

[EuiButtonIcon] Remove unused size modifiers/maps

9a3567e

+ convert enum types to new preferred syntax + move type enums to top of file

[EuiButtonIcon] Delete Sass files

15e7f28

[EuiButtonIcon] Add Storybook playground

7008979

+ configure all playgrounds to sort props by required first, then alphabetical

[EuiButtonIcon] Convert Enzyme tests to RTL

fdd5c55

changelog

8c1b3b0

cee-chen force-pushed the emotion/button-icon branch from a26c843 to 8c1b3b0 Compare June 20, 2023 15:32

cee-chen marked this pull request as ready for review June 20, 2023 15:52

cee-chen requested a review from breehall June 20, 2023 15:52

Update downstream snapshots

4806489

Fix Storybook icon loading

30a415c

- preload all icons into the cache at once in the preview - this increases Storybook's startup time somewhat significantly, but will probably be necessary until we resolve dynamic icons for SSR/Vite

breehall approved these changes Jun 21, 2023

View reviewed changes

cee-chen merged commit 34fff99 into elastic:main Jun 21, 2023

cee-chen deleted the emotion/button-icon branch June 21, 2023 18:40

cee-chen mentioned this pull request Jun 22, 2023

[Emotion] EuiButton #6388

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

[Emotion] Convert EuiButtonIcon #6844

[Emotion] Convert EuiButtonIcon #6844

cee-chen commented Jun 15, 2023 •

edited

Loading

kibanamachine commented Jun 20, 2023

kibanamachine commented Jun 20, 2023

kibanamachine commented Jun 20, 2023

kibanamachine commented Jun 21, 2023

breehall left a comment

breehall Jun 21, 2023

breehall Jun 21, 2023

breehall Jun 21, 2023

cee-chen commented Jun 21, 2023

cee-chen commented Jun 21, 2023

[Emotion] Convert EuiButtonIcon #6844

[Emotion] Convert EuiButtonIcon #6844

Conversation

cee-chen commented Jun 15, 2023 • edited Loading

Summary

QA

General checklist

Emotion checklist

Acceptance criteria

Checklists

kibanamachine commented Jun 20, 2023

kibanamachine commented Jun 20, 2023

kibanamachine commented Jun 20, 2023

kibanamachine commented Jun 21, 2023

breehall left a comment

Choose a reason for hiding this comment

breehall Jun 21, 2023

Choose a reason for hiding this comment

breehall Jun 21, 2023

Choose a reason for hiding this comment

breehall Jun 21, 2023

Choose a reason for hiding this comment

cee-chen commented Jun 21, 2023

cee-chen commented Jun 21, 2023

cee-chen commented Jun 15, 2023 •

edited

Loading