[Emotion] Add css prop to requiredProps#6886
Conversation
- to make it easier to examine Emotion className output in snapshots
- i.e., snapshots that don't render the super incredibly long Emotion wrapper
- To prevent the super verbose `<Emotion>` injection wrapper from rendering
- these should likely be converted to RTL tests, but honestly they also just need specific assertions and not just blanket snapshots - which would take too long at this point and I'm looking for minimal changes
- that's coming in from the new `requiredProps.css` approaches are: - converting to RTL `render` where possible - removing `..requiredProps` spreads for non `it renders` tests - `dive()`ing or `mount()`ing instead of `shallow`
| aria-label="aria-label" | ||
| aria-labelledby="generated-id" | ||
| class="euiButtonIcon euiAccordion__iconButton testClass1 testClass2 emotion-euiButtonIcon-xs-empty-text-euiAccordion__iconButton" | ||
| class="euiButtonIcon euiAccordion__iconButton testClass1 testClass2 emotion-euiButtonIcon-xs-empty-text-euiTestCss" |
There was a problem hiding this comment.
This snapshot actually exemplifies the bug I ran into when working on the new nav redesign. The euiTestCss passed to arrowProps.css is overriding EUI's euiAccordion__iconButton css instead of being merged correctly (which should look like euiAccordion__iconButton-euiTestCss).
This PR does not attempt to solve that problem (as this one is massive enough as it is, and I'm looking to break up the work into more easily reviewable chunks), but helps highlight the problem.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6886/ |
| test('is rendered', () => { | ||
| const component = shallow( | ||
| it('renders', () => { | ||
| const { container } = render( |
There was a problem hiding this comment.
Love to see more tests being converted to RTL 💯
|
This is a solid set of changes and I love that we can now easily see if I made a list of snapshots in this PR that have classes modified in a different way than adding |
tkajtoch
left a comment
There was a problem hiding this comment.
🚢 Approved! Feel free to merge after rebasing with latest main if there aren't more changes that may affect anything
|
Thanks Tomasz! Great eye on those changes as well, I do have fixes for all those snapshots coming up in the next PR. No issues/changes when merging latest except re-running |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6886/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6886/ |
Summary
This PR is part 1 of several PRs that attempt to better harden our testing around the
cssprop and how it reacts to consumers passing/merging in their own custom Emotioncsson top of EUI'scss.The primary change that it makes is adding a
cssprop torequiredProps(31b8ecf). Almost all other changes are simply snapshot updates reacting accordingly (across almost all components, since we usedrequiredPropsin almost all tests).Rendering custom
cssby default should hopefully help make it obvious what custom CSS will look like on top of EUI's CSS as well as spot issues where either custom CSS is not being applied or EUI's CSS is being accidentally overridden.This PR also incidentally strives to avoid snapshotting Emotion's injected wrapper (which occurs primarily in shallow/mounted snapshots) using a variety of techniques, depending on the complexity of the test and component (see commit messages for more detail).
I recommend reviewing by commit and skimming the 2nd commit with a million snapshot updates 😅
QA
General checklist
and cypress tests- [ ] A changelog entry exists and is marked appropriatelyN/A - tests only