Skip to content

[Emotion] Convert EuiProgress#5986

Merged
cee-chen merged 11 commits intoelastic:mainfrom
cee-chen:emotion/progress
Jun 23, 2022
Merged

[Emotion] Convert EuiProgress#5986
cee-chen merged 11 commits intoelastic:mainfrom
cee-chen:emotion/progress

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Jun 20, 2022 •
edited
Loading

Summary

Converts EuiProgress to Emotion and removes all Sass variables (no instances of usage in Kibana) and modifier classes (no usages in Kibana). I opted not to remove child euiProgress__data, euiProgress__label, and euiProgress__valueText classes for ease of hooking in for non-Emotion users, but we could easily remove them if we prefer it.

I strongly recommend following along by commit, as I did some clean-up along the way that are noted in commit messages.

Things to look out for when moving styles

  • Convert global Sass vars to JS version like sizing
  • Convert component-specific Sass vars to exported JS versions
  • Convert side specific padding, margin, and position to -inline and -block (Logical properties)
  • Use gap property to add margin between items if using flex
  • Reduce specificity where possible (usually by reducing class name chaining)
  • All animations or transitions are wrapped in euiCanAnimate (preferred euiCantAnimate instead for simplicity)

- [ ] Anything in the backlog that could be a quick fix for that component? Nothing I saw was a quick fix
- [ ] Can any still existing .js files be converted to TS?

QA

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

@thompsongl
Copy link
Copy Markdown
Contributor

thompsongl commented Jun 21, 2022

jenkins test this

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 21, 2022

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

@cee-chen cee-chen force-pushed the emotion/progress branch 2 times, most recently from 88177fc to c0f6d7b Compare June 21, 2022 20:08
@cee-chen cee-chen mentioned this pull request Jun 21, 2022
Closed
49 tasks
@cee-chen cee-chen force-pushed the emotion/progress branch 2 times, most recently from 5390651 to 111f321 Compare June 21, 2022 20:45
@cee-chen cee-chen requested review from cchaos and elizabetdev June 21, 2022 20:52
@cee-chen cee-chen marked this pull request as ready for review June 21, 2022 20:53
@cee-chen
Copy link
Copy Markdown
Contributor Author

cee-chen commented Jun 21, 2022

@cchaos and @miukimiu, I wasn't sure if just one or both of you would be interested in reviewing this PR so I tagged both of you, but please feel free to skip if it's just one.

Just to follow up from my demo today on passing props/JS variables to our Emotion styles - for completeness, this approach works well for EuiProgress which mostly handles conditional color CSS, but may not always work for all scenarios/components as Emotion will no longer statically generate all CSS together (conditional CSS is generated separately). This can be seen in the below screenshot, by the two different CSS blocks with the same selector:

I also investigated the nested keys approach (example in prod) that @cchaos mentioned but ended up deciding against it for EuiProgress specifically, for two reasons:

  1. It looks like Emotion doesn't output the parent key in its className. In a hello: { world: css`` } } example, the output className becomes css-{hash}-world, and hello isn't used at all. I didn't super love that because EuiProgress does need an actual native and indeterminate class.

  2. Using nested keys instead of functions doubled the amount of keys as well in this case:

{
  native: {
     vis0: // etc
     // ... through
     vis9: // etc
  },
  indeterminate: {
     vis0: // etc
     // ... through
     vis9: // etc
  },
}

which felt more repetitive than using JS logic. I'll definitely keep nested keys in mind for future cases however, I think for this specific component a flag just "felt" like it made the code cleaner. (Also we should def document nested keys in our Emotion wiki!)

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 21, 2022

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

1 similar comment
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 22, 2022

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

cee-chen added 10 commits June 22, 2022 09:08
@cee-chen
Add basic euiProgress styles
4004fab
@cee-chen
Convert euiProgress--native and euiProgress--indeterminate to Emotion
68b2a57 
+ move Amsterdam override CSS into Emotion

+ add prefers-reduced-motion accommodation
@cee-chen
Convert size modifiers to Emotion
c6907d6
@cee-chen
Convert position modifiers to Emotion
92f5ac7 
+ pass native/determinate JS var to Emotion styles to conditionally render native vs non native CSS

+ remove unnecessary `::moz-progress-bar` selector - that affects the value color, not the background color, and should not have been added
@cee-chen
Convert color modifiers to Emotion
a6712ae 
- create DRY helpers for repeating native vs indeterminate selectors

- start valueText styles while here

- remove unnecessary isNamedColor fn/typing in favor of single var

- fix playground control
@cee-chen
Add tests for JS styles logic
cb4acc5
@cee-chen
Convert euiProgress__data and other children to Emotion
765b732 
+ simplify CSS: apply text styling to parent __data wrapper as opposed to both children (truncation still needs to individually be on both children)

- simplify CSS: remove need for nested `+` selector by:
  - converting `padding-left` to `gap`
  - always setting text alignment to right (removes need for margin-left: auto)
  - always setting flex grow/flex shrink CSS (same visual effect as before)
@cee-chen
Update unit tests/snapshots
fc6ba27
@cee-chen
Remove all EuiProgress Sass files and variables
753037a
@cee-chen
Changelog
d725a28
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 22, 2022

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

@elizabetdev
Copy link
Copy Markdown
Contributor

elizabetdev commented Jun 22, 2022

@constancecchen, now that we're using emotion, is it possible to calculate a high contrast color for the valueText when the color is custom?

Screenshot 2022-06-22 at 18 04 43

@cee-chen

This comment was marked as outdated.

Sign in to view
elizabetdev
elizabetdev approved these changes Jun 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

Tested in Chrome, Safari, Edge, and Firefox. I also tested using the playground and looked at the code.

cchaos
cchaos reviewed Jun 23, 2022
View reviewed changes
@cee-chen cee-chen enabled auto-merge (squash) June 23, 2022 16:38
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 23, 2022

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

@cee-chen cee-chen merged commit 21a24c3 into elastic:main Jun 23, 2022
@cee-chen cee-chen deleted the emotion/progress branch June 23, 2022 17:21
@elizabetdev elizabetdev mentioned this pull request Jun 23, 2022
Merged
11 tasks
@1Copenut 1Copenut mentioned this pull request Jun 28, 2022
Merged
@cee-chen cee-chen mentioned this pull request Feb 20, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@cchaos cchaos cchaos left review comments

@elizabetdev elizabetdev elizabetdev 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.

5 participants

@cee-chen @thompsongl @kibanamachine @elizabetdev @cchaos