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 EuiStat #5968

Merged
merged 8 commits into from
Jun 15, 2022
Merged

[Emotion] Convert EuiStat #5968

merged 8 commits into from
Jun 15, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jun 14, 2022

Summary

Converts EuiStat to Emotion and also removes the CSS -- modifier classes (no usages in Kibana). The __ child classes I left in-place as there were a few usages in Kibana.

Things to look out for when moving styles

- [ ] 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

  • 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)
  • Anything in the backlog that could be a quick fix for that component? respect euiCanAnimate

QA

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

- this wasn't outputting anything meaningful as there is no `dark` key in `$titleColors`
+ convert text align to logical properties
- move all title logic together
- change ternary logic to props only instead of to entire title JSX
- move `euiStat__title` to flat className, similar to `euiStat__description`
- remove unnecessary string interpolation
- DRY out commonProps
- fix typing complaint on commonProps
@cee-chen cee-chen requested a review from breehall June 14, 2022 19:51
@cee-chen cee-chen marked this pull request as ready for review June 14, 2022 19:52
@cee-chen
Copy link
Member Author

@breehall Do you mind reviewing this when you have a quick sec? Should hopefully be pretty fast!

@kibanamachine
Copy link

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

    - to use actual options instead of static & potentially stale values
@kibanamachine
Copy link

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

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.

The conversion looks good! Pull and tested against the current version! LGTM

@cee-chen cee-chen merged commit 0d7da15 into elastic:main Jun 15, 2022
@cee-chen cee-chen deleted the emotion/stat branch June 15, 2022 20:33
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