Skip to content

[Emotion] Convert EuiHealth#5832

Merged
cee-chen merged 7 commits intoelastic:mainfrom
cee-chen:emotion/eui-health
Apr 26, 2022
Merged

[Emotion] Convert EuiHealth#5832
cee-chen merged 7 commits intoelastic:mainfrom
cee-chen:emotion/eui-health

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Apr 21, 2022

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

@cee-chen cee-chen mentioned this pull request Apr 21, 2022
Closed
49 tasks
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 21, 2022

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

1 similar comment
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 25, 2022

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
Copy Markdown
Contributor Author

cee-chen commented Apr 25, 2022

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

breehall
breehall reviewed Apr 25, 2022
View reviewed changes
breehall
breehall reviewed Apr 25, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

cee-chen commented Apr 25, 2022 •
edited
Loading

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
Copy Markdown
Contributor Author

cee-chen commented Apr 26, 2022

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

kibanamachine commented Apr 26, 2022

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
Copy Markdown
Contributor Author

cee-chen commented Apr 26, 2022

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

kibanamachine commented Apr 26, 2022

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

breehall
breehall approved these changes Apr 26, 2022
View reviewed changes
Copy link
Copy Markdown
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
@breehall breehall mentioned this pull request May 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@breehall breehall breehall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

emotion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cee-chen @kibanamachine @breehall