-
Notifications
You must be signed in to change notification settings - Fork 841
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
[Emotion] Convert EuiToken #6067
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
22c65e7
to
7ace739
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Thanks, @constancecchen for pushing 9ec7d9c. I tested and works perfectly! 👍🏽 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
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.
Removing the rectangle
shape is a breaking change. Have a couple very small requests, otherwise this LGTM
src/components/token/token.styles.ts
Outdated
color: ${iconColor}; | ||
|
||
|
||
&[class*='-light'] { |
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.
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.
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.
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.
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};
`
}
Thanks, @chandlerprall. I addressed your review with 720c0d4. Let me know if is there anything else to address. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
src/components/token/token.styles.ts
Outdated
color: ${iconColor}; | ||
|
||
|
||
&[class*='-light'] { |
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.
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};
`
}
Preview documentation changes for this PR: https://eui.elastic.co/pr_6067/ |
Thanks @chandlerprall, |
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.
Changes LGTM; I don't see any visual differences, including light+dark mode with various fills & colors
Summary
This PR converts EuiToken to Emotion.
xs
andsquare
shape the icon was touching the top and bottom borders so I added a small vertical padding (1px)border-radius
to therectangle
shape just because I think it looks better and it gets similar to thesquare
shapeThis 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
shapeI didn't see any rectangle shape usage. So I decided to make these tokens more aligned with the other tokens.
Checklist
[ ] Checked in mobile[ ] Props have proper autodocs and playground toggles[ ] Checked Code Sandbox works for any docs examples[ ] Checked for accessibility including keyboard-only and screenreader modes