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 EuiProgress #5986

Merged
merged 11 commits into from
Jun 23, 2022
Merged

Conversation

cee-chen
Copy link
Contributor

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

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
Contributor

jenkins test this

@kibanamachine
Copy link

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

@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

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

1 similar comment
@kibanamachine
Copy link

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

cee-chen added 10 commits June 22, 2022 09:08
+ move Amsterdam override CSS into Emotion

+ add prefers-reduced-motion accommodation
+ 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
- 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
+ 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)
@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor

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

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

@cee-chen cee-chen enabled auto-merge (squash) June 23, 2022 16:38
@kibanamachine
Copy link

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

5 participants