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] Memoize styles + opinionated clean up more components #8172

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

cee-chen
Copy link
Member

Summary

Part of #7561

TODO: probably split this up to 2 PRs

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

+ remove unnecessary function for styles w/ no theme token

+ inline JSX where possible
- disabled opacity is fairly static and can be DRYed out

- loading spinner: no styles being applied so this isn't necessary
+ remove passed `layout` arg in favor of static definitions

- rewrite vertical vs horizontal gutters utils to be slightly less confusing

+ replace `calc()` with `mathWithUnits()`
+ prefer inline JSX
+ rerun VRT to confirm no regressions. changes are minute/from font aliasing
- replace `EuiHideFor/ShowFor` usage with a breakpoint hook instead
+ remove unnecessary fragments

+ remove some unnecessary callback consts, just inline them if they're not reused and we're not going to memoize them
- `list_group_item` has too much conditional JSX to deal with right now, should likely be split up into subcomponents in the future

+ remove unnecessary style functions, just use a static obj instead

+ re-run VRT screenshots
+ reduce # of style fns for horizontal steps, and remove unnecessary onClick fn in favor of just inlining it (if we're not reusing or memoizing it)

+ update VRT
- instead of another div and repeated CSS, just use `> *` (which has zero specificity and is very overrideable) to bump the icon/avatar above the vertical line

+ inline `iconRender` and `ariaLabel`, it's not reused and not particularly increasing readability by being a const
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants