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 batch of basic/simple components #8171

Merged
merged 12 commits into from
Nov 21, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Nov 21, 2024

Summary

Part of #7561

This PR memoizes a swathe of basic euiComponentStyles(useEuiTheme()) ➡️ useEuiMemoizedStyles(euiComponentStyles) conversions. Some minor code syntax/lint type cleanups were made, but should not be major refactors.

VRT

While here, I took the opportunity to re-run our VRT screenshots per-component to both check for regressions and handle minute font-level updates (see #8074 (comment)).

@mgadewoll, I'd be curious to hear more thoughts on whether the VRT changes feel excessive to you. Could you try pulling this PR down and running, e.g. yarn loki update --storiesFilter EuiDragDropContext and see if it outputs a new screenshot for you or if it remains the same?

If that generates snapshot updates for you (argh), maybe we could consider changing the either the diffing engine that Loki uses, or the diff threshold?

QA

N/A, regression testing should be handled by VRT and/or snapshots

General checklist

  • Browser QA - N/A, should be handled by VRT
  • 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

+ update/rerun VRT + fix flaky VRT with `sleep` util (prev setTimeout wasn't quite working correctly/in the expected order)
+ update VRT scrreenshots
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen cee-chen marked this pull request as ready for review November 21, 2024 00:56
@cee-chen cee-chen requested a review from a team as a code owner November 21, 2024 00:56
@mgadewoll
Copy link
Contributor

@cee-chen I ran VRT locally for your PR on all components memoized in this PR and there were no new reference image updates 🎉
(based on a few quick checks, there are still other components that do result in updates around font rendering)

Copy link
Contributor

@mgadewoll mgadewoll 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 good to me! Thanks for tackling some of the leftover tech debt! 🧹

).toBe(true);
});

await sleep(150); // add a timeout to prevent differences due to animation
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL this existed! 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

lol same. We have so many handy utils in services that go unnoticed 😅

@cee-chen
Copy link
Member Author

Thanks for checking Lene! I'll continue updating VRT screenshots for components as I memoize their styles, and then we can do one final VRT update after which hopefully shouldn't be too much at once.

@cee-chen cee-chen merged commit 9b1b461 into elastic:main Nov 21, 2024
15 checks passed
@cee-chen cee-chen deleted the emotion/memoization-3 branch November 21, 2024 16:19
@mgadewoll mgadewoll mentioned this pull request Nov 22, 2024
4 tasks
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