-
Notifications
You must be signed in to change notification settings - Fork 837
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
[EuiSpacer] Converts EuiSpacer
to Emotion
#5812
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
EuiSpacer
to EmotionEuiSpacer
to Emotion
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Adding QA checklist specific to CSS-in-JS conversions: Things to look out for when moving styles
QA
|
Looks like there's a couple downstream snapshots that need to be updated - LMK if you're still updating this PR @snide or if you want me to grab it! |
@constancecchen ty. Updated (I think I got them, |
src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap
Outdated
Show resolved
Hide resolved
src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap
Outdated
Show resolved
Hide resolved
I'm investigating the EuiContextMenu flaking in a separate PR btw 🤞 |
return <div className={classes} {...rest} />; | ||
const cssStyles = [styles.euiSpacer, styles[size]]; | ||
|
||
return <div className={classes} css={cssStyles} {...rest} />; |
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.
Per the #5685 (comment) checklist, do we want to add a renderWithStyles()
test to spacer.test.tsx
? See the EuiMark example:
eui/src/components/mark/mark.test.tsx
Lines 16 to 17 in e01a8eb
describe('EuiMark', () => { | |
renderWithStyles(<EuiMark>Marked</EuiMark>); |
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.
@constancecchen in that comment also mentions adding shouldRenderCustomStyles
. Is that correct?
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.
Yeah! I just added that helper yesterday after I made this comment. We would want both (or just 1, if/when we decide to get renderWithStyles
either fixed or removed)
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
@@ -1,18 +0,0 @@ | |||
$spacerSizes: ( |
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.
Looks like 10 instances in Kibana are targeting this map from the converted JSON. How would we like to handle that upgrade process?
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.
In all seriousness, we'd probably convert over directly to theme.eui.euiSize{size}
similar to what I did here: elastic/kibana@64651f1
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.
For @snide to explain, why we can't have the consumers now import from euiSpacerStyles
Current:
padding: ${(props) => props.theme.eui.spacerSizes.xl};
which evaluated essentially to:
padding: $euiSizeXXL;
which rendered as:
padding: 40px
If they tried to grab the emotion styles
euiSpacerStyles(euiTheme)[xxl]
They get a string:
'height: 40px;'
So they can't do:
padding: ${euiSpacerStyles(euiTheme)[xxl]};
as it would render as:
padding: height: 40px;
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.
In this specific component's case, it's actually quite an easy conversion as the spacers were exactly mapped to the same named size variable. So they easily just not import from Spacer but just the global sizing variable instead.
Others may not be so easy, and may depend on how exactly they're trying to match components.
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.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Yes, it's correct. the EuiMarkDown snapshot was unnecessary and was removed in favor of an assertion. I copied the same approach Chandler took in #5815 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Co-authored-by: Elizabet Oliveira <elizabet.oliveira@elastic.co>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@cchaos Good suggestions. I think we're good to go now? |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/ |
Summary
Converts
EuiSpacer
to emotion.Video diff
https://snid.es/2022APR/Glcc87PjPKbXzX92.mp4
Checklist
[ ] Props have proper autodocs and playground togglesAdded documentationChecked Code Sandbox works for any docs examplesChecked for accessibility including keyboard-only and screenreader modesUpdated the Figma library counterpart