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 EuiContextMenu, EuiContextMenuPanel, and EuiContextMenuItem #7312

Merged
merged 25 commits into from
Oct 25, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 24, 2023

Summary

closes #6392

Converting this component to CSS-in-JS also will also address elastic/kibana#169571 and elastic/kibana#169313 (comment) (once merged into Kibana main).

This is kind of a lot to review (especially tests, which I went in and edited judiciously), so I think I'd recommend sticking to QA only for reviewing this PR (as regression/QA testing is more important here)

Also while here, I removed an extra unnecessary <div> wrapper within EuiContextMenuItem. Div wrappers left me an orphan in a dark alley at a young age, and this is how I seek justice.

QA

  • Go to https://eui.elastic.co/pr_7312/#/navigation/context-menu
  • Regression test that all context menus look the same as production
    • ⚠️ NOTE: There is one (intentional) change, which is that context menu titles are now slightly less tall than before. This is expected, and due to changes in line-height from new euiTitle Emotion styles.
  • Go to https://eui.elastic.co/pr_7312/#/layout/bottom-bar
  • Click the first toggle example
  • Click the Help button
  • Confirm that the resulting context menu popover renders with the correct text color in both light and dark mode

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A

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 - None
  • Any test/query selectors that will need to be updated
  • 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)

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 No playground

Unit tests

  • shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
  • Enzyme tests converted to RTL

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
  • [ ] 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 No overrides

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) - Note: removed an entire child node/div wrapper
  • SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana. - No usages

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

- not used anwhere as in either EUI or Kibana
- inline it since it's pretty basic
+ remove unused `--next`/`--previous` classes
…PanelTitle`

- by dogfooding `EuiContextMenuItem`

- this requires updating `EuiContextMenuItem`'s logic to be able to render a `div` if necessary

- we can now remove the `padding` variables, since `EuiContextMenuItem` is now the only one that uses it
- due to the previous change, they'd otherwise render as `div`s and not register within the tab order
+ syntax cleanup - prefer `const` and `&&` over `let` and `if`
- we're applying these styles directly via Emotion in the child components instead of at the parent
- add `shouldRenderCustomStyles`

- convert all tests to RTL

- convert snapshots to assertions where it makes sense to do so
- Fix class assertion failures caused by new Emotion classes (convert `have.attr, class`->`have.class`)

- Fix focus failures caused by items without `href`s or `onClick`s rendering as `div`s
- due to the way the component is currently set up with `onAnimationEnd`, setting a 0s animation duration is the easiest way to reduce motion
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen cee-chen marked this pull request as ready for review October 24, 2023 22:21
@cee-chen cee-chen requested a review from a team as a code owner October 24, 2023 22:21
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I ran through your manual QA criteria on all four evergreen browsers in light and dark themes. No issues except the slightly shorter context menu title as noted in the description.

@cee-chen cee-chen merged commit ab6290d into elastic:main Oct 25, 2023
8 checks passed
@cee-chen cee-chen deleted the emotion/context_menu branch October 25, 2023 21:33
cee-chen added a commit to elastic/kibana that referenced this pull request Nov 3, 2023
`v89.1.0`⏩`v90.0.0`

The majority of changes in this PR come from:

- **EuiContextMenu** being converted to Emotion
(elastic/eui#7312). If your usage of
`EuiContextMenu` was significantly affected, we recommend pulling down
this PR and QAing it locally.

- `defaultProps` being removed from some very widespread components,
particularly **EuiButton**, in anticipation of React's upcoming
deprecation.
(elastic/eui@b7dc9b4)
**NOTE**: This only affected Enzyme snapshots, and did not affect
production behavior.

[Commits](https://github.com/elastic/kibana/pull/170179/commits) have
been broken up by component changes as well as types of changes.

---

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

- Updated the `eventColor` prop on `EuiCommentEvent` to apply the color
to the entire comment header.
([#7288](elastic/eui#7288))
- Updated `EuiBasicTable` and `EuiInMemoryTable` to support a new
controlled selection API: `selection.selected`
([#7321](elastic/eui#7321))

**Bug fixes**

- Fixed controlled `EuiFieldNumbers` not correctly updating native
validity state ([#7291](elastic/eui#7291))
- Fixed `EuiListGroupItem` to pass `style` props to the wrapping `<li>`
element alongside `className` and `css`. All other props will be passed
to the underlying content.
([#7298](elastic/eui#7298))
- Fixed `EuiListGroupItem`'s non-transitioned transform on hover/focus
([#7298](elastic/eui#7298))
- Fixed `EuiDataGrid`s with `gridStyle.stripes` sometimes showing buggy
row striping after being sorted
([#7301](elastic/eui#7301))
- Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to not conflict with
`gridStyle.stripes` if dynamically updated
([#7301](elastic/eui#7301))
- Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to support multiple
space-separated classes
([#7301](elastic/eui#7301))
- Fixed `EuiInputPopover` not calling `onPanelResize` callback prop
([#7305](elastic/eui#7305))
- Fixed `EuiDualRange` incorrectly positioning highlights when rendered
with `showInput="inputWithPopover"`
([#7305](elastic/eui#7305))
- Fixed `EuiTabs` incorrectly wrapping text when it should instead
either scroll or truncate
([#7309](elastic/eui#7309))
- `EuiContextMenu` now renders text colors correctly when used within an
`EuiBottomBar` ([#7312](elastic/eui#7312))
- Fixed the width of `EuiSuperDatePicker`'s Absolute date picker
([#7313](elastic/eui#7313))
- Fixed `EuiDataGrid` cells visually cutting off overflowing content a
little too quickly ([#7320](elastic/eui#7320))

**Deprecations**

- Deprecated `EuiBasicTable` and `EuiInMemoryTable`'s ref `setSelection`
API. Use the new `selection.selected` API instead.
([#7321](elastic/eui#7321))

**Breaking changes**

- Removed `EuiPageTemplate_Deprecated`, `EuiPageSideBar_Deprecated`, and
`EuiPageContent*_Deprecated`
([#7265](elastic/eui#7265))
- Removed the `ghost` color option from `EuiButton`, `EuiButtonEmpty`,
and `EuiButtonIcon`. Use an `<EuiThemeProvider colorMode="dark">`
wrapper and `color="text"` instead.
([#7296](elastic/eui#7296))

**Dependency updates**

- Updated `refractor` to v3.6.0
([#7127](elastic/eui#7127))
- Updated `rehype-raw` to v5.1.0
([#7127](elastic/eui#7127))
- Updated `vfile` to v4.2.1
([#7127](elastic/eui#7127))

**Accessibility**

- `EuiContextMenu` now correctly respects reduced motion preferences
([#7312](elastic/eui#7312))
- `EuiAccordion`s no longer attempt to focus child content when the
accordion is externally opened via `forceState`, but will continue to
focus expanded content when users click the toggle button.
([#7314](elastic/eui#7314))

**CSS-in-JS conversions**

- Converted `EuiContextMenu`, `EuiContextMenuPanel`, and
`EuiContextMenuItem` to Emotion; Removed `$euiContextMenuWidth`
([#7312](elastic/eui#7312))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Emotion] EuiContextMenu
4 participants