-
Notifications
You must be signed in to change notification settings - Fork 842
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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
cee-chen
force-pushed
the
emotion/context_menu
branch
from
October 24, 2023 21:30
3c72253
to
7c2c46f
Compare
Preview staging links for this PR:
|
💚 Build Succeeded
|
1Copenut
approved these changes
Oct 25, 2023
There was a problem hiding this 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.
This was referenced Oct 30, 2023
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 withinEuiContextMenuItem
. Div wrappers left me an orphan in a dark alley at a young age, and this is how I seek justice.QA
title
s are now slightly less tall than before. This is expected, and due to changes inline-height
from neweuiTitle
Emotion styles.Help
buttonGeneral checklist
- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)Emotion checklist
Kibana usage
**/target, **/*.snap, **/*.storyshot
for less noise) foreui{Component}
(case sensitive) to find:{euiComponent}-
(case sensitive) to check for usage of modifier classes - NoneeuiBadge
class on a div instead of simply using theEuiBadge
component)General
className(s)
read as expected in snapshots and browsers[ ] Checked component playgroundNo playgroundUnit tests
shouldRenderCustomStyles()
test was added and passes with parent component and any nestedchildProps
(e.g.tooltipProps
)Sass/Emotion conversion process
$euiSize
toeuiTheme.size.base
)or convertedcomponent-specific Sass vars/mixinsto exported JS versions[ ] Simplifiedcalc()
tomathWithUnits
if possible (if mixing different unit types, this may not be possible)[ ] Added an@warn
deprecation message within theglobal_styling/mixins/{component}.scss
file[ ] All SCSS overrides have been removed from the Amsterdam themeNo overridesCSS tech debt
euiCanAnimate
gap
property to add margin between items if using flex-inline
and-block
logical properties (check inline styles as well as CSS)DOM Cleanup
euiComponent
,euiComponent__child
) - Note: removed an entire child node/div wrapperExtras/nice-to-have
[ ] 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