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.

Sign up for GitHub

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

Jump to bottom

[Emotion][perf] Memoize loading components #7649

Merged
merged 4 commits into from
Apr 4, 2024
Merged

[Emotion][perf] Memoize loading components #7649

merged 4 commits into from
Apr 4, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Apr 3, 2024 •
edited by mgadewoll
Loading

Summary

Part of #7561 efforts

This PR:

  • Memoizes all EuiLoading* components
  • Refactors how EuiLoadingChart styles are written/generated to use static :nth-child CSS instead of dynamic JS styles (98ebe26). Also removes its --mono className modifier - no usages in Kibana
  • All other components were fairly straightforward

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes. Also checked with Reduced motion setting
  • Docs site QA - N/A
  • Code quality checklist - N/A, all tests should pass
  • Release checklist - N/A, skipping a changelog as this is mostly tech debt and shouldn't affect either consumers or end-users
  • Designer checklist - N/A

cee-chen added 4 commits April 3, 2024 12:22
@cee-chen
Memoize loading spinner
c1468a5 
- not memoizing the inline style - usage is fairly low / it's not worth it
+ add story for easier testing
@cee-chen
Memoize loading logo + remove unnecessary extra style fn
4ac9a04 
- it's not super complex, so just pair it with the main styles
@cee-chen
Elastic logo - remove unnecessary styles fn
6da43a5 
- doesn't need theme tokens so can just be a static obj
@cee-chen
Refactor/memoize EuiLoadingChart
98ebe26 
- prefer static nth-child CSS over a dynamically generated function (since it's not like the bar count is customizable via prop)

- inline the bar JSX and replace for loop with Array.from

+ fix the duplicate loading i18n (should be using the shared string with the same copy)
@cee-chen cee-chen mentioned this pull request Apr 3, 2024
Open
22 tasks
@cee-chen cee-chen marked this pull request as ready for review April 3, 2024 20:25
@cee-chen cee-chen requested a review from a team as a code owner April 3, 2024 20:25
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@tkajtoch tkajtoch self-requested a review April 4, 2024 11:46
tkajtoch
tkajtoch approved these changes Apr 4, 2024
View reviewed changes
Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

Changes look and work great!

@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 4, 2024

Thanks Tomasz!

@cee-chen cee-chen merged commit bed1865 into elastic:main Apr 4, 2024
16 of 17 checks passed
@cee-chen cee-chen deleted the emotion/memoization-loading branch April 4, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkajtoch tkajtoch tkajtoch approved these changes

Assignees
No one assigned
Labels
emotion performance skip-changelog tech debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cee-chen @kibanamachine @elasticmachine @tkajtoch