[Emotion] Convert EuiLink #5856

MichaelMarcialis · 2022-04-29T21:13:38Z

Summary

This PR converts the majority of the EuiLink component to CSS-in-JS. The only outstanding SCSS is the portion that references the euiFocusRing mixin, which has yet to be converted to Emotion. Once that has been converted, we can go back and remove that last bit of SCSS. TODO comments have been added to echo that statement in the appropriate files.

I did not add a deprecation warning to the global and Amsterdam euiLink mixins, as it is being used by other yet-to-be-converted components in EUI. However, if the deprecation warning should be added regardless, please let me know.

Big thanks to @cchaos for the guidance!

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?~~
~~[ ] All animations or transitions are wrapped in euiCanAnimate~~

QA

Does the output CSS match the previous CSS / as expected
Does the rendered class read as expected

…sion

cchaos · 2022-05-02T17:15:31Z

@thompsongl Something odd is happening on CI when it's trying to create TS defs on this PR for EuiLink:

12:35:15 Generating typescript definitions file
12:35:15 eui.d.ts(7225,31): error TS2307: Cannot find module '@elastic/eui/src/components/link/' or its corresponding type declarations.

I couldn't find anything specifically that would have affected this in the changeset. Can you take a look?

miukimiu

Thanks, @MichaelMarcialis for doing this conversion.

There are some things that can be improved and I added some suggestions.

I'm just not completely sure about the best way of doing the mixins. @constancecchen or @cchaos can take a look at the mixins to see if they can be improved?

src/components/link/link.tsx

src/global_styling/mixins/_link.ts

src/components/link/link.styles.ts

src/global_styling/mixins/_link.ts

src/components/link/link.styles.ts

thompsongl · 2022-05-04T14:37:21Z

Something odd is happening on CI when it's trying to create TS defs on this PR for EuiLink

I have an idea, but everything passes locally. So I'm just going to push commits here until it's resolved.

kibanamachine · 2022-05-04T15:21:03Z

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

thompsongl · 2022-05-04T15:33:48Z

10:31:40 Generating typescript definitions file
10:33:06 ✔ Finished generating definitions

6be8eea resolved the CI failure

…sion # Conflicts: # src/global_styling/mixins/index.ts

MichaelMarcialis · 2022-05-06T14:46:32Z

Thanks so much for the detailed review, @miukimiu! Please forgive all my rookie mistakes 😬

I believe I've addressed all your comments, but let me know if I've missed anything. While making these changes, two questions came to mind regarding modifier styles:

It looks like we're using a BEM-style naming convention for the child selector styles (block__element), but not for modifier styles (block--modifier). Is that correct?
Similarly, we only show examples of declaring child selector styles at the top of the document (as mentioned in your comments). But this is not necessary for modifier styles, right?

Thanks again!

kibanamachine · 2022-05-06T15:10:04Z

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

cchaos

Thanks @MichaelMarcialis! I found just a couple other things and some nits.

Regarding BEM naming...

As you can see from the snapshot out of the className we don't have to manually apply BEM naming for modifiers:

Which is why we always make sure the name of the component is the first style key, in this case euiLink. Then Emotion will apply the subsequent style keys as -modifier to the className.

css-uj6g16-euiLink-button-colorStyles-accent

The same applies for children. The first key we grab and pass to the children needs to be the euiComponentName__childName so that it's output first as you can see in the external icon use:

class="css-jqzood-euiLink__externalIcon"

upcoming_changelogs/5856.md

src/components/link/link.tsx

cchaos · 2022-05-02T16:03:55Z

src/components/link/link.styles.ts

+      ${colorStyles(euiTheme.colors.primaryText)}
+    `,
+    subdued: css`
+      ${colorStyles(euiTheme.colors.subdued)}


Not to self, I think this token name should change to subduedText to be explicit.

src/global_styling/mixins/index.ts

src/components/link/link.tsx

src/components/link/link.styles.ts

…sion

MichaelMarcialis · 2022-05-06T17:49:39Z

Thanks, @cchaos! I've updated the PR with your feedback. Let me know if I missed anything.

kibanamachine · 2022-05-06T18:18:37Z

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

cchaos

🙌 LGTM! Just one last nit and maybe a conflict resolve gone bad? Otherwise, should be good to merge

src/components/link/link.styles.ts

src/components/markdown_editor/markdown_editor.test.tsx

kibanamachine · 2022-05-06T19:35:11Z

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

Taken care of

miukimiu

LGTM! 🎉

kibanamachine · 2022-05-09T12:13:45Z

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

MichaelMarcialis added 7 commits April 29, 2022 13:15

initial attempt to convert euiLink to emotion

80d95bf

split color helper into separate style blocks

34bccf4

fix broken disabled styles

d098936

clean up

78cf4c6

update tests and snapshots

d58464c

remove styling vars

ba662d8

Merge branch 'main' of github.com:elastic/eui into 5685/euiLinkConver…

9193db8

…sion

MichaelMarcialis added the emotion label Apr 29, 2022

MichaelMarcialis mentioned this pull request Apr 29, 2022

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

Closed

49 tasks

add changelog entry

6a4aad3

MichaelMarcialis marked this pull request as ready for review April 29, 2022 21:30

miukimiu self-requested a review May 2, 2022 15:46

one more snapshot update

d85e897

MichaelMarcialis self-assigned this May 4, 2022

miukimiu previously requested changes May 4, 2022

View reviewed changes

remove unused arg and import; fix snapshot

6be8eea

MichaelMarcialis added 3 commits May 5, 2022 16:42

Merge branch 'main' of github.com:elastic/eui into 5685/euiLinkConver…

8020a08

…sion # Conflicts: # src/global_styling/mixins/index.ts

apply elizabet’s suggestions

0a71e48

update snapshots

b63f956

MichaelMarcialis requested a review from miukimiu May 6, 2022 14:40

cchaos suggested changes May 6, 2022

View reviewed changes

MichaelMarcialis added 3 commits May 6, 2022 13:42

apply caroline's suggestions

258898f

Merge branch 'main' of github.com:elastic/eui into 5685/euiLinkConver…

4cfbb9b

…sion

update snapshots

9c094c5

MichaelMarcialis requested a review from cchaos May 6, 2022 17:48

cchaos approved these changes May 6, 2022

View reviewed changes

src/components/link/link.styles.ts Show resolved Hide resolved

src/components/markdown_editor/markdown_editor.test.tsx Show resolved Hide resolved

add children comment

2ebd0f5

miukimiu approved these changes May 9, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 5685/euiLinkConversion

69db45f

MichaelMarcialis merged commit a22356c into elastic:main May 9, 2022

MichaelMarcialis deleted the 5685/euiLinkConversion branch May 9, 2022 13:24

breehall mentioned this pull request May 16, 2022

Upgrade EUI to v58.0.0 elastic/kibana#132257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert EuiLink #5856

[Emotion] Convert EuiLink #5856

MichaelMarcialis commented Apr 29, 2022 •

edited

Loading

cchaos commented May 2, 2022

miukimiu left a comment

thompsongl commented May 4, 2022

kibanamachine commented May 4, 2022

thompsongl commented May 4, 2022

MichaelMarcialis commented May 6, 2022

kibanamachine commented May 6, 2022

cchaos left a comment

cchaos May 2, 2022

MichaelMarcialis commented May 6, 2022

kibanamachine commented May 6, 2022

cchaos left a comment

kibanamachine commented May 6, 2022

miukimiu left a comment

kibanamachine commented May 9, 2022

[Emotion] Convert EuiLink #5856

[Emotion] Convert EuiLink #5856

Conversation

MichaelMarcialis commented Apr 29, 2022 • edited Loading

Summary

Things to look out for when moving styles

QA

cchaos commented May 2, 2022

miukimiu left a comment

Choose a reason for hiding this comment

thompsongl commented May 4, 2022

kibanamachine commented May 4, 2022

thompsongl commented May 4, 2022

MichaelMarcialis commented May 6, 2022

kibanamachine commented May 6, 2022

cchaos left a comment

Choose a reason for hiding this comment

cchaos May 2, 2022

Choose a reason for hiding this comment

MichaelMarcialis commented May 6, 2022

kibanamachine commented May 6, 2022

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented May 6, 2022

miukimiu left a comment

Choose a reason for hiding this comment

kibanamachine commented May 9, 2022

MichaelMarcialis commented Apr 29, 2022 •

edited

Loading