[EuiSpacer] Converts `EuiSpacer` to Emotion #5812

snide · 2022-04-19T14:39:39Z

Summary

Converts EuiSpacer to emotion.

Video diff
https://snid.es/2022APR/Glcc87PjPKbXzX92.mp4

Checklist

kibanamachine · 2022-04-19T15:27:59Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

kibanamachine · 2022-04-19T16:10:24Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

cee-chen · 2022-04-19T16:20:14Z

Adding QA checklist specific to CSS-in-JS conversions:

Things to look out for when moving styles

~~- [ ] Anything in the backlog that could be a quick fix for that component?~~

Convert global Sass vars to JS version like sizing

~~- [ ] Convert component-specific Sass vars to exported JS versions~~
~~- [ ] Convert side specific padding, margin, and position to -inline and -block (Logical properties)~~
~~- [ ] Reduce specificity where possible (usually by reducing class name chaining)~~
~~- [ ] Use gap property to add margin between items if using flex~~
~~- [ ] Can any still existing .js files be converted to TS?~~

QA

Do className, css, and style props work properly when passed by the consumer
Does the rendered class read as expected

cee-chen · 2022-04-19T16:20:24Z

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!

snide · 2022-04-19T16:27:25Z

@constancecchen ty. Updated (I think I got them, EuiContextMenu continues to be a little flakey for me and failed locally)

src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap

cee-chen · 2022-04-19T16:51:00Z

I'm investigating the EuiContextMenu flaking in a separate PR btw 🤞

cee-chen · 2022-04-19T16:51:48Z

src/components/spacer/spacer.tsx

-  return <div className={classes} {...rest} />;
+  const cssStyles = [styles.euiSpacer, styles[size]];
+
+  return <div className={classes} css={cssStyles} {...rest} />;


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>);

@constancecchen in that comment also mentions adding shouldRenderCustomStyles. Is that correct?

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)

kibanamachine · 2022-04-19T16:58:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

cchaos · 2022-04-19T19:55:21Z

src/components/spacer/_spacer.scss

@@ -1,18 +0,0 @@
-$spacerSizes: (


Looks like 10 instances in Kibana are targeting this map from the converted JSON. How would we like to handle that upgrade process?

In all seriousness, we'd probably convert over directly to theme.eui.euiSize{size} similar to what I did here: elastic/kibana@64651f1

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;

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.

OK I wrote up some explanation here. Ultimately this will remain tricky, but at least it's something we can point to. @cchaos can you give it a read to see if it reads clear?

5781933

kibanamachine · 2022-04-19T20:07:03Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

cchaos · 2022-04-19T20:11:27Z

I pushed a quick fix to the docs:

kibanamachine · 2022-04-19T20:40:31Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

cchaos · 2022-04-19T20:52:26Z

Wait.. is this right??

upcoming_changelogs/5812.md

cee-chen · 2022-04-19T21:19:55Z

Wait.. is this right??

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

kibanamachine · 2022-04-19T21:52:54Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

kibanamachine · 2022-04-20T15:52:59Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

Co-authored-by: Caroline Horn <[email protected]>

wiki/consuming.md

kibanamachine · 2022-04-20T16:30:25Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

Co-authored-by: Elizabet Oliveira <[email protected]>

wiki/consuming.md

kibanamachine · 2022-04-20T17:06:03Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

Co-authored-by: Caroline Horn <[email protected]>

snide · 2022-04-20T19:44:19Z

@cchaos Good suggestions. I think we're good to go now?

kibanamachine · 2022-04-20T20:14:25Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5812/

Dave Snider added 2 commits April 19, 2022 10:38

Converts spacer to Emotion

173bb89

fix medium size to base size, add cl

3ee3375

snap snap

01f59b9

snide changed the title ~~[DRAFT] Converts spacer to Emotion~~ Converts spacer to Emotion Apr 19, 2022

snide marked this pull request as ready for review April 19, 2022 15:43

snide mentioned this pull request Apr 19, 2022

[Meta][CSS-in-JS] Component conversions #5685

Closed

49 tasks

snide changed the title ~~Converts spacer to Emotion~~ Converts EuiSpacer to Emotion Apr 19, 2022

snide changed the title ~~Converts EuiSpacer to Emotion~~ [EuiSpacer] Converts EuiSpacer to Emotion Apr 19, 2022

snap snap

8f3a14d

snide commented Apr 19, 2022

View reviewed changes

src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap Outdated Show resolved Hide resolved

snide commented Apr 19, 2022

View reviewed changes

src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap Outdated Show resolved Hide resolved

cee-chen reviewed Apr 19, 2022

View reviewed changes

Remove downstream mounted snapshots

05f4d1c

cchaos reviewed Apr 19, 2022

View reviewed changes

Fixed docs

a9b8836

cchaos reviewed Apr 19, 2022

View reviewed changes

upcoming_changelogs/5812.md Outdated Show resolved Hide resolved

cee-chen added 2 commits April 19, 2022 14:23

Merge remote-tracking branch 'upstream/main' into emotion_spacer

bb4273e

Add custom styles test

8f5b878

wiki context for theming

5781933

Update upcoming_changelogs/5812.md

473e1c4

Co-authored-by: Caroline Horn <[email protected]>

elizabetdev reviewed Apr 20, 2022

View reviewed changes

wiki/consuming.md Outdated Show resolved Hide resolved

Update wiki/consuming.md

aea9142

Co-authored-by: Elizabet Oliveira <[email protected]>

cchaos reviewed Apr 20, 2022

View reviewed changes

wiki/consuming.md Show resolved Hide resolved

wiki/consuming.md Show resolved Hide resolved

snide and others added 2 commits April 20, 2022 15:43

Update wiki/consuming.md

de7f9c0

Co-authored-by: Caroline Horn <[email protected]>

Update wiki/consuming.md

3b4a83a

Co-authored-by: Caroline Horn <[email protected]>

cchaos approved these changes Apr 20, 2022

View reviewed changes

snide merged commit 810c0a0 into elastic:main Apr 20, 2022

snide deleted the emotion_spacer branch April 20, 2022 22:28

1Copenut mentioned this pull request Apr 28, 2022

Upgrade EUI to v55.1.2 elastic/kibana#131203

Merged

	describe('EuiMark', () => {
	renderWithStyles(<EuiMark>Marked</EuiMark>);

[EuiSpacer] Converts EuiSpacer to Emotion #5812

[EuiSpacer] Converts EuiSpacer to Emotion #5812

Uh oh!

Conversation

snide commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Apr 19, 2022

Uh oh!

kibanamachine commented Apr 19, 2022

Uh oh!

cee-chen commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things to look out for when moving styles

QA

Uh oh!

cee-chen commented Apr 19, 2022

Uh oh!

snide commented Apr 19, 2022

Uh oh!

Uh oh!

Uh oh!

cee-chen commented Apr 19, 2022

Uh oh!

cee-chen Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

elizabetdev Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 19, 2022

Uh oh!

cchaos Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

cchaos Apr 19, 2022

Choose a reason for hiding this comment

Current:

If they tried to grab the emotion styles

Uh oh!

cchaos Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snide Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 19, 2022

Uh oh!

cchaos commented Apr 19, 2022

Uh oh!

kibanamachine commented Apr 19, 2022

Uh oh!

cchaos commented Apr 19, 2022

Uh oh!

Uh oh!

cee-chen commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Apr 19, 2022

Uh oh!

kibanamachine commented Apr 20, 2022

Uh oh!

Uh oh!

kibanamachine commented Apr 20, 2022

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Apr 20, 2022

Uh oh!

snide commented Apr 20, 2022

Uh oh!

[EuiSpacer] Converts `EuiSpacer` to Emotion #5812

[EuiSpacer] Converts `EuiSpacer` to Emotion #5812

snide commented Apr 19, 2022 •

edited

Loading

cee-chen commented Apr 19, 2022 •

edited

Loading

cchaos Apr 19, 2022 •

edited

Loading

cee-chen commented Apr 19, 2022 •

edited

Loading