[Emotion] Fix consumer css overriding (instead of merging with) EUI css in child props #6896
Conversation
- since I'll be adding a few more needed options here shortly
- will be adding a few more here shortly, as well as extending the fn, and it's nice not to have to continously pass `options` around
- will be adding another util here shortly that will be using it, and 3 times is enough to DRY it out
cee-chen
force-pushed
the
fix-css-merging-2
branch
from
04a35a8 to
f10d78d
Compare
- the next util we're adding will use it
- this is the main fix/guard/detection for the buggy behavior found in this PR - it acts by rendering the baseline component w/ no custom `css`, grabbing the Emotion className generated, and ensuring the custom `css` is appended to the end of it instead of gobbling it - this behavior primarily affects nested `childProps` components, as Emotion does not auto-merge `css` props that isn't the immediately returned element / isn't at the top level of the component
- Emotion handles merging automatically, so this essentially mimics end behavior but in test
cee-chen
force-pushed
the
fix-css-merging-2
branch
from
f10d78d to
5920a02
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6896/ |
…s` overriding EUI css
- fix childProps `className` not being correctly merged - fix `css` not being correctly merged - rewrite `style` merge/overrides a bit more succinctly
+ DRY out checkable typeof logic
- extracting `css` out and setting it to the end of the array ensures it will always correctly come after EUI styles + misc cleanup / better organization of code + add extra non-panelled test
- behavior is working, but because EuiTour has conflates some props with `panelProps`, we need to isolate and not pass any extra `css` (in `requiredProps`) other than what is automatically tested
+ update `EuiResizablePanel` to use this new `wrapper` options, which both significantly DRYs out behavior as well as allowing `shouldRenderCustomStyles` to clone props correctly
- bogart the new wrapper API to preload `appendIconComponentCache`, otherwise the EUI class comparisons encounter race conditions with the `isLoading` CSS
…shouldRenderCustomStyles` - EuiImage renders multiple copies of an image wrapper on the page, so we need a specific way of targeting which DOM node to get the right Emotion classes from + convert remaining tests to RTL as cleanup/tech debt + remove skipped test entirely
- As noted, Emotion does not auto-merge non-top-level props so we need to do it manually for css merging to work correctly. This means a lot of disables - we're probably better off linting for the presence of `shouldRenderCustomStyles` instead, which actually tests for working Emotion CSS
cee-chen
force-pushed
the
fix-css-merging-2
branch
from
a89092b to
a5dbaad
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6896/ |
| let euiCss = ''; | ||
| classes?.forEach((classString: string) => { | ||
| if (!classString.startsWith('css-')) return; | ||
|
|
||
| const matches = classString.match(/css-[\d\w-]{4,12}-(?<euiCss>eui.+)/); | ||
| if (matches?.groups?.euiCss) { | ||
| euiCss = matches.groups.euiCss; | ||
| } | ||
| }); | ||
| return euiCss; |
There was a problem hiding this comment.
Just wanted to highlight a known edge case in this className parser - if, for some reason, a component has multiple separate Emotion classNames on it, e.g.
<div className="notEmotionClass css-12345-euiFoo css-67890-euiBar">This matcher will only look at the last Emotion className rather than at both, which may not be the one that the test actually wants.
That being said, the likelihood of that is slim to none for most default EUI classes. The only time a separate Emotion className gets added separately (instead of merged) is when the vanilla JS @emotion/css is used instead of @emotion/react (which handles the merge behavior). And we typically only use @emotion/css for the rare times we're directly dealing with vanilla JS nodes, e.g. portals or EuiDataGrid, etc, and those typically will not already have an Emotion className on them.
All in all, I thought it was enough of an edge case that I didn't want to bother spending more time to code in a specific handler for that scenario - if we come across it in the future, I'll expand shouldRenderCustomStyles at that point.
There was a problem hiding this comment.
💯 I reviewed each commit individually, and got a good understanding of what the PR was accomplishing. The comments and updated docs were extremely helpful. I'll add an enhancement ticket to write a new ESLint rule that checks for shouldRenderCustomStyles tests.
|
Thanks a million Trevor, you rock! |
`[email protected]` ⏩ `83.1.0` --- ## [`83.1.0`](https://github.com/elastic/eui/tree/v83.1.0) - Added `placeholder` prop to `EuiInlineEdit` ([#6883](elastic/eui#6883)) - Added `sparkles` glyph to `EuiIcon` ([#6898](elastic/eui#6898)) **Bug fixes** - Fixed Safari-only bug for single-line row `EuiDataGrid`s, where cell actions on hover would overlap instead of pushing content to the left ([#6881](elastic/eui#6881)) - Fixed `EuiButton` not correctly merging in passed `className`s with its base `.euiButton` class ([#6887](elastic/eui#6887)) - Fixed `EuiIcon` not correctly passing the `style` prop custom `img` icons ([#6888](elastic/eui#6888)) - Fixed multiple components with child props (e.g. `buttonProps`, `iconProps`, etc.) unsetting EUI's Emotion styling if custom `css` was passed to the child props object ([#6896](elastic/eui#6896)) **CSS-in-JS conversions** - Converted `EuiHeader` and `EuiHeaderLogo` to Emotion ([#6878](elastic/eui#6878)) - Removed Sass variables `$euiHeaderDarkBackgroundColor`, `$euiHeaderBorderColor`, and `$euiHeaderBreadcrumbColor` ([#6878](elastic/eui#6878)) - Removed Sass mixin `@euiHeaderDarkTheme` ([#6878](elastic/eui#6878))
Summary
This PR fixes components with nested
childPropsprops that were overriding EUI's Emotion css instead of merging in both EUI and consumer css.It also refactors and hardens
shouldRenderCustomStylesto throw/error if consumer Emotion styles/classes aren't merging as expected: 9be0f79I've added wiki documentation (5920a02) that explains why this behavior occurs and how to avoid it. It specifically only affects child elements and not top-level parent elements because Emotion handles merging
cssarrays for us for top-level components, but does not perform that auto-merge behavior for nested children within components.I know this PR is on the larger side - the majority of the code to review is around the
shouldRenderCustomStylesutil, the individual component commits are thankfully on the much quicker side of things. As always, I strongly recommend following along by commit if possible.Note about the lint rule removal
I removed our custom lint rule that checks for
css={}props before{...spreadOperators}(947fd6d). Unfortunately, Emotion merging simply doesn't work the way we thought it did when we wrote that rule. The order in which thecssprop isn't what matters other than the lastcssprop taking precedence over previouscssprops.Again, the primary issue is that Emotion auto-merges top-level component props only but not nested child elements. So for child elements, we need to manually merge
cssand make surecsscomes after the spread operator, which means that this rule is too opinionated to use in its current state.CC @1Copenut who wrote the above lint rule - we may want to consider writing another rule instead that checks that all components have a
shouldRenderCustomStylestest, as that is generally going to be the most robust method of ensuringcssis being merged correctly going forward.Related PRs:
Part 1 of this work: #6886
Old PRs with
childPropsfixes:csswas being set, but unfortunately did not test that consumer css was merged with existing EUI css instead of overriding it.QA
General checklist