[Emotion] Convert EuiToken by elizabetdev · Pull Request #6067 · elastic/eui

elizabetdev · 2022-07-20T15:54:21Z

Summary

This PR converts EuiToken to Emotion.

I also improved a few comments in the code and in the props table. Some of them in my opinion were difficult to understand.
When in size xs and square shape the icon was touching the top and bottom borders so I added a small vertical padding (1px)
Added a border-radius to the rectangle shape just because I think it looks better and it gets similar to the square shape

This is only for testing purposes:

Fixed EuiIconTokenStruct

Why isn't the token getting centered? Well... It seems that it was incorrectly exported from Figma/Sketch. So I fixed the design and recompiled the icon.

Changed tokens that were defaulting to `rectangle` shape

I didn't see any rectangle shape usage. So I decided to make these tokens more aligned with the other tokens.

Checklist

Checked in both light and dark modes
[ ] Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
[ ] Props have proper autodocs and playground toggles
Added documentation
~~[ ] Checked Code Sandbox works for any docs examples~~
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
~~[ ] Checked for accessibility including keyboard-only and screenreader modes~~
Updated the Figma library counterpart
A changelog entry exists and is marked appropriately

kibanamachine · 2022-07-20T16:22:11Z

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

kibanamachine · 2022-07-21T15:31:21Z

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

kibanamachine · 2022-07-22T17:59:37Z

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

kibanamachine · 2022-07-25T12:50:34Z

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

…` size.

kibanamachine · 2022-07-25T13:28:33Z

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

kibanamachine · 2022-07-25T16:19:03Z

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

kibanamachine · 2022-07-25T16:59:18Z

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

kibanamachine · 2022-07-25T17:55:20Z

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

src/components/token/token.tsx

kibanamachine · 2022-07-25T19:21:05Z

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

kibanamachine · 2022-07-25T20:32:14Z

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

elizabetdev · 2022-07-26T11:02:42Z

Thanks, @constancecchen for pushing 9ec7d9c. I tested and works perfectly! 👍🏽

kibanamachine · 2022-07-27T12:57:13Z

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

kibanamachine · 2022-07-27T16:02:59Z

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

chandlerprall

Removing the rectangle shape is a breaking change. Have a couple very small requests, otherwise this LGTM

chandlerprall · 2022-07-27T17:32:47Z

src/components/token/token.styles.ts

+    color: ${iconColor};
+
+
+    &[class*='-light'] {


Are these attribute selectors necessary? If I'm reading this correctly, since color mode is passed in the style generation should know which form is needed and can return just those styles.

When I started doing this conversion I also got confused with these light and dark props and class names. They correspond to the fills.

It is not related to the color mode. When a token is light it looks like this:

When is dark:

When is none:

How do you feel about passing the fill prop through, keeping the css tied to the prop value instead of matching against the generated selector. Looks like it would require creating a token.types.ts file to avoid a circular dependency.

${ fill === 'light' ? ` color: ${lightColor}; background-color: ${backgroundLightColor}; box-shadow: inset 0 0 0 1px ${boxShadowColor}; ` : ` color: ${darkColor}; background-color: ${backgroundDarkColor}; ` }

src/components/token/token.tsx

elizabetdev · 2022-07-28T09:36:55Z

Thanks, @chandlerprall. I addressed your review with 720c0d4. Let me know if is there anything else to address.

kibanamachine · 2022-07-28T10:01:35Z

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

upcoming_changelogs/6067.md

chandlerprall · 2022-07-28T16:34:53Z

src/components/token/token.styles.ts

+    color: ${iconColor};
+
+
+    &[class*='-light'] {


How do you feel about passing the fill prop through, keeping the css tied to the prop value instead of matching against the generated selector. Looks like it would require creating a token.types.ts file to avoid a circular dependency.

${ fill === 'light' ? ` color: ${lightColor}; background-color: ${backgroundLightColor}; box-shadow: inset 0 0 0 1px ${boxShadowColor}; ` : ` color: ${darkColor}; background-color: ${backgroundDarkColor}; ` }

kibanamachine · 2022-07-28T19:08:26Z

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

elizabetdev · 2022-07-29T10:28:26Z

Thanks @chandlerprall,

I'm now passing the fill to getTokenColor() 0b3f9f6
Added the reactangle shape back to the TokenShape type f385963
Renamed and converted the examples to TS 6eeff25

chandlerprall

Changes LGTM; I don't see any visual differences, including light+dark mode with various fills & colors

elizabetdev added 3 commits June 20, 2022 13:35

Removing token rectangle

e69311a

Small color fix

8b0d081

Merge remote-tracking branch 'upstream/main' into emotion-token

1f7aeab

elizabetdev added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Jul 20, 2022

Putting rectangle shape back. Moving custom colors to style tag.

a2c06d9

elizabetdev added 2 commits July 22, 2022 14:09

Merge remote-tracking branch 'upstream/main' into emotion-token

01e1e50

Better comments. Better custom example. Renaming a few variables.

ba9b552

Deleting *.scss files

227a7ea

Adding new example with a glyph. Adding required logic for `EuiIcon…

51621ae

…` size.

elizabetdev added 3 commits July 25, 2022 15:20

Replacing last custom example with faceHappy icon

0f34784

Adding customColor to className

d026372

Putting customColor or tokenColor at the end of the class name

7ace739

elizabetdev force-pushed the emotion-token branch from 22c65e7 to 7ace739 Compare July 25, 2022 15:12

elizabetdev added 3 commits July 25, 2022 16:26

Better comment

4fdd5aa

Again. :P Better prop comment.

ffaa309

Trying to enhance comments

1a80cea

Centering EuiIconTokenStruct. Fix border-radius for xs size

147ea92

Fix xs rectangle shape

7c46954

elizabetdev added 3 commits July 25, 2022 19:01

Adding CL

d27eb4c

Adding shouldRenderCustomStyles

0794321

Removed styling variables

5f2583a

elizabetdev added the emotion label Jul 25, 2022

Removed unused key

b1128a8

elizabetdev marked this pull request as ready for review July 25, 2022 18:59

elizabetdev commented Jul 25, 2022

View reviewed changes

src/components/token/token.tsx Outdated Show resolved Hide resolved

cee-chen force-pushed the emotion-token branch from 3c09f3f to 1c003ec Compare July 25, 2022 20:05

Remove need for lodash defaults

9ec7d9c

cee-chen force-pushed the emotion-token branch from 1c003ec to 9ec7d9c Compare July 25, 2022 20:06

elizabetdev mentioned this pull request Jul 26, 2022

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

Closed

49 tasks

elizabetdev added 2 commits July 27, 2022 13:23

Updated iconSize with logicalSizeCSS

fe08564

Adding logicalCSS for width

42afc2b

Merge remote-tracking branch 'upstream/main' into emotion-token

9572d52

chandlerprall suggested changes Jul 27, 2022

View reviewed changes

Addressing PR review

720c0d4

chandlerprall reviewed Jul 28, 2022

View reviewed changes

elizabetdev added 4 commits July 28, 2022 17:37

Merge remote-tracking branch 'upstream/main' into emotion-token

c605348

Passing the fill prop to getTokenColor

0b3f9f6

Adding rectangle shape back to type TokenShape

f385963

Converting examples to .tsx

6eeff25

elizabetdev requested a review from chandlerprall July 29, 2022 10:28

chandlerprall approved these changes Jul 29, 2022

View reviewed changes

elizabetdev merged commit fd38ef3 into elastic:main Jul 29, 2022

thompsongl mentioned this pull request Aug 15, 2022

Upgrade EUI to v62.2.3 elastic/kibana#138837

Merged

Conversation

elizabetdev commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fixed EuiIconTokenStruct

Changed tokens that were defaulting to rectangle shape

Checklist

Uh oh!

kibanamachine commented Jul 20, 2022

Uh oh!

kibanamachine commented Jul 21, 2022

Uh oh!

kibanamachine commented Jul 22, 2022

Uh oh!

kibanamachine commented Jul 25, 2022

Uh oh!

kibanamachine commented Jul 25, 2022

Uh oh!

kibanamachine commented Jul 25, 2022

Uh oh!

kibanamachine commented Jul 25, 2022

Uh oh!

kibanamachine commented Jul 25, 2022

Uh oh!

Uh oh!

kibanamachine commented Jul 25, 2022

Uh oh!

kibanamachine commented Jul 25, 2022

Uh oh!

elizabetdev commented Jul 26, 2022

Uh oh!

kibanamachine commented Jul 27, 2022

Uh oh!

kibanamachine commented Jul 27, 2022

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

chandlerprall Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

elizabetdev Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

chandlerprall Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elizabetdev commented Jul 28, 2022

Uh oh!

kibanamachine commented Jul 28, 2022

Uh oh!

Uh oh!

chandlerprall Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 28, 2022

Uh oh!

elizabetdev commented Jul 29, 2022

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elizabetdev commented Jul 20, 2022 •

edited

Loading

Changed tokens that were defaulting to `rectangle` shape