Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Emotion] Convert EuiKeyPadMenuItem #7118

Merged
merged 15 commits into from
Aug 24, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Aug 23, 2023

Summary

closes #6653

As always, I recommend following along by commit, as I included reasoning for various changes in commit messages, as well as misc cleanup items

🔲 Kibana follow-up items

  • There's just one class modifier usage being used in a test selector that will be need to be updated in the next upgrade. IMO, usage is low enough that it's worth nuking all selectors

QA

  • Compare staging against production, and confirm the component looks the same as before
    • NOTE: There will likely be some minute typography differences due to Emotion containing more up to date font and line height style changes. It should not be major however.
  • gh checkout pr 7118
  • Go to http://localhost:6006/?path=/story/euikeypadmenuitem--playground
  • Play with various props (particularly permutations of isSelected, isDisabled, and checkable and confirm general UI/UX is working and non-broken

General checklist

  • Checked in both light and dark modes
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Emotion checklist

Kibana usage

  • 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:
  • 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 - going to just look at the entire className string instead
  • Any test/query selectors that will need to be updated - Yes, one
  • 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

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)
    • Already added

Sass/Emotion conversion process

  • 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
  • All SCSS files have been removed from the component(s) directory
  • All SCSS overrides have been removed from the Amsterdam theme
  • [ ] Any dependent components are identified in a new issue

CSS tech debt

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

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.

Extras/nice-to-have

  • Reduced specificity where possible (usually by reducing nesting and class name chaining)
    • Note that this wasn't fully possible for all scenarios, e.g. when hovering a parent affects a child, or for affecting the fill color of SVG icons
  • [ ] 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

- `@include euiFont` is no longer necessary, our Emotion global resets cover `button`
- doesn't appear to be used as of Amsterdam
NOTE: I vastly simplified the color functions down to basic tokens. As far as I could tell the mix/contrast fns weren't really doing anything anyway, and currently nothing that's statically generated can correctly handle contrast ratios combined with transparent backgrounds

+ tweak obj syntax to handle disabled + selected state (optimizing for generated Emotion className output)

+ remove unnecessary extra CSS for checkable selected, it's the exact same styles
@cee-chen cee-chen changed the title [Emotion] Convert EuiKeyPadMenuItem [Emotion] Convert EuiKeyPadMenuItem Aug 23, 2023
@kibanamachine
Copy link

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

+ temporary workaround for Sass specificity

+ DRY out shared props between checkbox & radio components
- the functions otherwise get redefined and recalled on every rerender, which seems like a waste

+ memoize child styles, since it's in the dependency array
@cee-chen cee-chen requested review from a team and breehall August 23, 2023 22:52
@cee-chen cee-chen marked this pull request as ready for review August 23, 2023 22:52
@kibanamachine
Copy link

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

Love the optimizations within this PR! Storybook demonstrated the isSelected, isDisabled, and checkable props / permutations well.

[Just a note] This commit where you made the decision to simplify the color token -we should continue to do this where possible! I know it won't be easy in every case, but I think its beneficial as we improve accessibility (color contrast specifically) within EUI.

Comment on lines 11 to 15
// Calculate the right text contrast ratio based on focus transparency
$backgroundColorSimulated: mix($euiColorEmptyShade, $euiColorPrimary, $euiFocusTransparencyPercent);
color: makeHighContrastColor($euiColorPrimaryText, $backgroundColorSimulated);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I vastly simplified the color functions down to basic tokens.

Given the recent conversation in Slack about color contrast and transparency, I think this was a good decision.

Copy link
Member Author

Choose a reason for hiding this comment

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

[Just a note] This commit where you made the decision to simplify the color token -we should continue to do this where possible! I know it won't be easy in every case, but I think its beneficial as we improve accessibility (color contrast specifically) within EUI.

Thanks for pointing this out, Bree! Just to follow up on the transparency topic as well, it's worth mentioning that dynamic color contrast calculations with transparency might be coming some day to native CSS functions, but right now there's no static contrast calculations that can be done with transparency and any dynamic calculations would require investigation.

Comment on lines 217 to 224
const sharedProps = {
id: itemId,
className: 'euiKeyPadMenuItem__checkableInput',
css: childStyles.euiKeyPadMenuItem__checkableInput,
checked: isSelected,
disabled: isDisabled,
name,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the DRY object

Comment on lines +204 to +207
const childStyles = useMemo(
() => euiKeyPadMenuItemChildStyles(euiTheme),
[euiTheme]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

the functions otherwise get redefined and recalled on every rerender

Great thinking & optimization

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I've sometimes wondered whether all our emotion style objs should be memoized. We haven't run into any perf issues or complaints yet though, so I'm probably overthinking it 😅

@cee-chen cee-chen merged commit 6ce9cfd into elastic:main Aug 24, 2023
2 checks passed
@cee-chen cee-chen deleted the emotion/keypad-menu-item branch August 24, 2023 21:39
jbudz pushed a commit to elastic/kibana that referenced this pull request Sep 5, 2023
## PR change summary

`v87.2.0`⏩`v88.1.0`

⚠️ The biggest thing to QA in this PR is several **breaking changes** to
`EuiDescriptionList`.

### Description list `columnWidths` prop

This PR introduces a new `columnWidths` prop and removes several Kibana
instances of custom CSS overrides to title/description column widths.

The primary motivation behind this is not just to reduce custom CSS, but
also because v88.0.0 introduced an underlying CSS change of `column`
description lists to using [`display: grid`
CSS](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout).
The new prop allows us to support existing description list custom
widths while not requiring Kibana developers to understand or write grid
CSS (except for 1 or two scenarios around `max-width`).

⚠️ **No user-facing UI around column widths should have regressed as a
result of these changes. If they have, please let us know in this PR.**

### Description list gutter size changes

The prop `gutterSize` has been renamed to `rowGutterSize` and the
default size is now `s` instead of `m`.

The change to `s` from `m` means there is an **expected** smaller gap
between list items (see below screenshots):

**Current `EuiDescriptionList` with default `rowGutterSize="s"`**
<img width="753" alt=""
src="https://github.com/elastic/kibana/assets/934879/c17aef28-ed3b-40c4-84c3-396e788b13bb">

**Prior `EuiDescriptionList` with default `gutterSize="m"`**
<img width="721" alt=""
src="https://github.com/elastic/kibana/assets/934879/84d5f5a2-8fa6-4f99-9dc0-73fd143aa1e1">

If Kibana teams prefer to keep the previous `m` gutter for their
instances of `EuiDescriptionList`, you have a couple of options:

1. Let EUI team know in the PR and we can set usage back to what it was
before
2. Set `rowGutterSize="m"` yourselves manually

---

## [`88.1.0`](https://github.com/elastic/eui/tree/v88.1.0)

- Added `font.defaultUnits` theme token. EUI component font sizes
default to `rem` units - this token allows consumers to configure this
to `px` or `em` ([#7133](elastic/eui#7133))
- Updated `EuiDescriptionList` with new `columnWidths` prop
([#7146](elastic/eui#7146))

**Bug fixes**

- Fixed `EuiDataGrid`'s keyboard shortcuts popover display
([#7146](elastic/eui#7146))

**CSS-in-JS conversions**

- Renamed `useEuiFontSize()`'s `measurement` option to `unit` for
clarity ([#7133](elastic/eui#7133))

## [`88.0.0`](https://github.com/elastic/eui/tree/v88.0.0)

- Updated `EuiDescriptionList` with a new `columnGutterSize` prop
([#7062](elastic/eui#7062))

**Deprecations**

- Deprecated `EuiSuggest`. We recommend using `EuiSelectable` or
`EuiComboBox` instead
([#7122](elastic/eui#7122))
- Deprecated `EuiControlBar`. We recommend using `EuiBottomBar` instead
([#7122](elastic/eui#7122))
- Deprecated `EuiColorStops`. We recommend copying the component to your
application if necessary
([#7122](elastic/eui#7122))
- Deprecated `EuiNotificationEvent`. We recommend copying the component
to your application if necessary
([#7122](elastic/eui#7122))

**Breaking changes**

- Renamed `EuiDescriptionList`'s `gutterSize` prop to `rowGutterSize`
([#7062](elastic/eui#7062))
- `EuiDescriptionList`'s `rowGutterSize` prop now defaults to a size of
`s` (was previously `m`)
([#7062](elastic/eui#7062))

**Accessibility**

- Fixed the dark mode colors of inline `EuiDescriptionListTitle`s to
meet WCAG color contrast requirements
([#7062](elastic/eui#7062))

**CSS-in-JS conversions**

- Converted `EuiKeyPadMenuItem` to Emotion; Removed `$euiKeyPadMenuSize`
and `$euiKeyPadMenuMarginSize`
([#7118](elastic/eui#7118))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Nikita Indik <nikita.indik@elastic.co>
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Sep 19, 2023
## PR change summary

`v87.2.0`⏩`v88.1.0`

⚠️ The biggest thing to QA in this PR is several **breaking changes** to
`EuiDescriptionList`.

### Description list `columnWidths` prop

This PR introduces a new `columnWidths` prop and removes several Kibana
instances of custom CSS overrides to title/description column widths.

The primary motivation behind this is not just to reduce custom CSS, but
also because v88.0.0 introduced an underlying CSS change of `column`
description lists to using [`display: grid`
CSS](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout).
The new prop allows us to support existing description list custom
widths while not requiring Kibana developers to understand or write grid
CSS (except for 1 or two scenarios around `max-width`).

⚠️ **No user-facing UI around column widths should have regressed as a
result of these changes. If they have, please let us know in this PR.**

### Description list gutter size changes

The prop `gutterSize` has been renamed to `rowGutterSize` and the
default size is now `s` instead of `m`.

The change to `s` from `m` means there is an **expected** smaller gap
between list items (see below screenshots):

**Current `EuiDescriptionList` with default `rowGutterSize="s"`**
<img width="753" alt=""
src="https://github.com/elastic/kibana/assets/934879/c17aef28-ed3b-40c4-84c3-396e788b13bb">

**Prior `EuiDescriptionList` with default `gutterSize="m"`**
<img width="721" alt=""
src="https://github.com/elastic/kibana/assets/934879/84d5f5a2-8fa6-4f99-9dc0-73fd143aa1e1">

If Kibana teams prefer to keep the previous `m` gutter for their
instances of `EuiDescriptionList`, you have a couple of options:

1. Let EUI team know in the PR and we can set usage back to what it was
before
2. Set `rowGutterSize="m"` yourselves manually

---

## [`88.1.0`](https://github.com/elastic/eui/tree/v88.1.0)

- Added `font.defaultUnits` theme token. EUI component font sizes
default to `rem` units - this token allows consumers to configure this
to `px` or `em` ([elastic#7133](elastic/eui#7133))
- Updated `EuiDescriptionList` with new `columnWidths` prop
([elastic#7146](elastic/eui#7146))

**Bug fixes**

- Fixed `EuiDataGrid`'s keyboard shortcuts popover display
([elastic#7146](elastic/eui#7146))

**CSS-in-JS conversions**

- Renamed `useEuiFontSize()`'s `measurement` option to `unit` for
clarity ([elastic#7133](elastic/eui#7133))

## [`88.0.0`](https://github.com/elastic/eui/tree/v88.0.0)

- Updated `EuiDescriptionList` with a new `columnGutterSize` prop
([elastic#7062](elastic/eui#7062))

**Deprecations**

- Deprecated `EuiSuggest`. We recommend using `EuiSelectable` or
`EuiComboBox` instead
([elastic#7122](elastic/eui#7122))
- Deprecated `EuiControlBar`. We recommend using `EuiBottomBar` instead
([elastic#7122](elastic/eui#7122))
- Deprecated `EuiColorStops`. We recommend copying the component to your
application if necessary
([elastic#7122](elastic/eui#7122))
- Deprecated `EuiNotificationEvent`. We recommend copying the component
to your application if necessary
([elastic#7122](elastic/eui#7122))

**Breaking changes**

- Renamed `EuiDescriptionList`'s `gutterSize` prop to `rowGutterSize`
([elastic#7062](elastic/eui#7062))
- `EuiDescriptionList`'s `rowGutterSize` prop now defaults to a size of
`s` (was previously `m`)
([elastic#7062](elastic/eui#7062))

**Accessibility**

- Fixed the dark mode colors of inline `EuiDescriptionListTitle`s to
meet WCAG color contrast requirements
([elastic#7062](elastic/eui#7062))

**CSS-in-JS conversions**

- Converted `EuiKeyPadMenuItem` to Emotion; Removed `$euiKeyPadMenuSize`
and `$euiKeyPadMenuMarginSize`
([elastic#7118](elastic/eui#7118))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Nikita Indik <nikita.indik@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Emotion] EuiKeyPadMenu (EuiKeyPadMenuItem Component Only 2/2)
4 participants