Skip to content
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

Merged
merged 29 commits into from
Jul 29, 2022
Merged

[Emotion] Convert EuiToken #6067

merged 29 commits into from
Jul 29, 2022

Conversation

miukimiu
Copy link
Contributor

@miukimiu miukimiu commented Jul 20, 2022

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

before-after-demo@2x

This is only for testing purposes:

before-after@2x

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.

tokenStruct@2x

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.

defaults@2x

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
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@miukimiu miukimiu marked this pull request as ready for review July 25, 2022 18:59
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@miukimiu
Copy link
Contributor Author

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a 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

color: ${iconColor};


&[class*='-light'] {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2022-07-27 at 22 06 02

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

Screenshot 2022-07-27 at 22 15 33

When is dark:

Screenshot 2022-07-27 at 22 14 49

When is none:

Screenshot 2022-07-27 at 22 17 48

Copy link
Contributor

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};
        `
    }

src/components/token/token.tsx Outdated Show resolved Hide resolved
src/components/token/token.tsx Outdated Show resolved Hide resolved
@miukimiu
Copy link
Contributor Author

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

@kibanamachine
Copy link

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

upcoming_changelogs/6067.md Outdated Show resolved Hide resolved
color: ${iconColor};


&[class*='-light'] {
Copy link
Contributor

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};
        `
    }

@kibanamachine
Copy link

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

@miukimiu
Copy link
Contributor Author

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

Copy link
Contributor

@chandlerprall chandlerprall left a 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

@miukimiu miukimiu merged commit fd38ef3 into elastic:main Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants