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 EuiHealth #5832

Merged
merged 7 commits into from
Apr 26, 2022
Merged

Conversation

cee-chen
Copy link
Contributor

Summary

Converts EuiHealth to CSS-in-JS and also removes the extra CSS text size modifier classes - I grepped through Kibana and confirmed that no tests are using said classes as hooks; only storybook snapshots will need to be updated.

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 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?

  • Convert component-specific Sass vars to exported JS versions

QA

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

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@cee-chen cee-chen requested a review from breehall April 25, 2022 21:11
@cee-chen
Copy link
Contributor Author

@breehall Do you mind taking a look and reviewing this PR? Thanks a million! 🙏

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

This looks great! One thing I noticed was when comparing the PR Preview of the EUI docs to the current version of EUI, the line height seems to be a tad bit smaller.
It looks like the line-height in this PR is 1.4286rem, and in the current version of EUI it's 1.71429rem. This isn't a huge change, but I just wanted to bring it up just in case the change wasn't intended.

D9E09E13-CEEE-4E6D-993B-F28D4D971B55

@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 25, 2022

Ahh, thanks for catching this! I missed that the new euiFontSize() mixin sets line-height whereas the old Sass only sets font-size.

@cchaos, do you mind chiming in here? Is setting line-height alongside font-size the correct approach, or should I opt to use euiFontSizeFromScale() instead of euiFontSize()?

@cee-chen
Copy link
Contributor Author

I'm just going to go ahead and change euiFontSize to euiFontSizeFromScale

@cee-chen cee-chen force-pushed the emotion/eui-health branch from 4ac060c to 7c68b07 Compare April 26, 2022 18:02
@cee-chen cee-chen requested a review from breehall April 26, 2022 18:17
@kibanamachine
Copy link

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

@cee-chen cee-chen force-pushed the emotion/eui-health branch from 0f03e0f to 882da2a Compare April 26, 2022 19:10
@cee-chen
Copy link
Contributor Author

I totally misinterpreted what prod was doing with line-height. It looks like we should be keeping that in and using the new line height base determined by euiFontSize. I've reverted the commits around that

@kibanamachine
Copy link

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

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

Looks good to me! ✨

@cee-chen cee-chen enabled auto-merge (squash) April 26, 2022 19:47
@cee-chen cee-chen merged commit b91f094 into elastic:main Apr 26, 2022
@cee-chen cee-chen deleted the emotion/eui-health branch April 26, 2022 19:58
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.

3 participants