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][perf] Memoize low-impact components (A-C) #7635

Merged
merged 11 commits into from
Apr 2, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 29, 2024

Summary

Basically every component that isn't on the medium impact list (#7561) and was easier to do a quick skim through. Some of the commits/components go a little extra on the cleanup though 😅 so as always, I recommend following by commit.

QA

  • Quick smoke test of all the affected components

General checklist

  • Browser QA - Skipping cross-browser testing in favor of smoke testing
  • Docs site QA - N/A
  • Code quality checklist - N/A
  • Release checklist - N/A, skipping changelog due to this being perf tech debt that shouldn't impact consumers
  • Designer checklist - N/A

cee-chen added 10 commits March 29, 2024 09:09
- make styles a static obj instead of a function
+ remove unnecessary props - consumers should apply those to the underlying cloned component instead
- remove `let`s in favor of `useMemo`s/conditional JSX

- clean up/reorganize variables

- make `visColors` a static const instead of per-component

- fix test with image url not actually rendering the correct inline style, + remove background color/color calculations on avatars with images - they aren't using them (add a default gray bg instead)
- remove unnecessary `paddingSizeToClassNameMap` - not used anywhere in Kibana

- remove unnecessary `{ position }` CSS in favor of just writing it in the position styles
@cee-chen cee-chen marked this pull request as ready for review March 29, 2024 23:14
@cee-chen cee-chen requested a review from a team as a code owner March 29, 2024 23:14
Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

I had a couple of questions about whether or not we need to add release notes for some of the changes, and a couple of questions about tests. Otherwise, looks good to me and nothing blocking.

src/components/aspect_ratio/aspect_ratio.tsx Show resolved Hide resolved
src/components/avatar/avatar.tsx Show resolved Hide resolved
src/components/bottom_bar/bottom_bar.tsx Show resolved Hide resolved
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen
Copy link
Contributor Author

cee-chen commented Apr 2, 2024

Going to go ahead and merge this since comments have been answered or addressed, but feel free to continue the conversation if you have other thoughts or there's anything more I can clarify!

@cee-chen cee-chen merged commit 68439d3 into elastic:main Apr 2, 2024
7 checks passed
@cee-chen cee-chen deleted the emotion/memoization-2 branch April 2, 2024 15:40
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.

4 participants