Skip to content

[Emotion][perf] Memoize EuiFlyout components/styles#7636

Merged
cee-chen merged 4 commits intoelastic:mainfrom
cee-chen:emotion/memoization-flyouts
Apr 3, 2024
Merged

[Emotion][perf] Memoize EuiFlyout components/styles#7636
cee-chen merged 4 commits intoelastic:mainfrom
cee-chen:emotion/memoization-flyouts

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Mar 29, 2024 •
edited
Loading

Summary

Part of #7561 efforts

This PR:

  • Memoizes all EuiFlyout and flyout subcomponent styles
  • Pulls out the flyout close button into its own internal subcomponent (its styles alone felt complex enough to warrant a separate file)
  • Attempts to inline or memoize more JSX in EuiFlyout (feedback requested here - see below comment 🤔)

QA

  • EuiFlyout should look the same as before, particularly the close button (try toggling closeButtonPosition or hideCloseButton for example)

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 - N/A
  • Code quality checklist - No tests updated, but should pass
  • Release checklist
    • Skipping a changelog as this is mostly tech debt and shouldn't affect either consumers or end-users
  • Designer checklist - N/A

@cee-chen cee-chen added skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt emotion performance labels Mar 29, 2024
@cee-chen cee-chen force-pushed the emotion/memoization-flyouts branch from 8f602e8 to ab78501 Compare March 29, 2024 20:05
cee-chen added 3 commits March 29, 2024 14:18
@cee-chen
[EuiFlyoutBody][EuiFlyoutFooter] More complex style memoization
f430317 
- reduce CSS arrays where they aren't needed

- fix Emotion name that was missing an `eui-` prefix and was snapshotting a hash
@cee-chen
[EuiFlyoutCloseButton] DRY out to internal subcomponent
cd9a7d2 
- there's enough going on here in terms of styling etc that it deserves to be separated from the main parent flyout file
@cee-chen
[EuiFlyout] Memoize styles + reduce inline JSX
3e3130c 
- not 100% sure if this is worth it but tbh maybe it's easier to read anyway? :)
@cee-chen cee-chen force-pushed the emotion/memoization-flyouts branch from ab78501 to 3e3130c Compare March 29, 2024 21:19
@cee-chen cee-chen mentioned this pull request Mar 29, 2024
Open
22 tasks
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Mar 29, 2024

Preview staging links for this PR:

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 29, 2024

💚 Build Succeeded

History

@cee-chen cee-chen marked this pull request as ready for review April 2, 2024 18:59
@cee-chen cee-chen requested a review from a team as a code owner April 2, 2024 19:00
cee-chen
cee-chen commented Apr 2, 2024
View reviewed changes
mgadewoll
mgadewoll approved these changes Apr 3, 2024
View reviewed changes
Copy link
Copy Markdown
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.

🚢 🐈‍⬛ LGTM!

@cee-chen cee-chen merged commit 3ca575b into elastic:main Apr 3, 2024
@cee-chen cee-chen deleted the emotion/memoization-flyouts branch April 3, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgadewoll mgadewoll mgadewoll approved these changes

Assignees

No one assigned

Labels

emotion performance skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cee-chen @kibanamachine @elasticmachine @mgadewoll