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
Performance: Avoid rerendering the sitehub unnecessarily #55818
Conversation
@@ -72,7 +72,6 @@ export default function Layout() { | |||
useCommonCommands(); | |||
useBlockCommands(); | |||
|
|||
const hubRef = useRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is useless.
@@ -226,13 +225,6 @@ export default function Layout() { | |||
animate={ headerAnimationState } | |||
> | |||
<SiteHub | |||
variants={ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this variants is a prop of SiteHub and not applied directly within the component.
@@ -32,7 +32,7 @@ import { unlock } from '../../lock-unlock'; | |||
|
|||
const HUB_ANIMATION_DURATION = 0.3; | |||
|
|||
const SiteHub = forwardRef( ( { isTransparent, ...restProps }, ref ) => { | |||
const SiteHub = memo( ( { isTransparent, ...restProps } ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you removed variants
props. I guess we can just use isTransparent
and className
props. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Riad. I think with clear list of props it would be "harder" to break memoization for a component.
Size Change: -30 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense and I like the prop cleanup ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my manual tests this update does not affect the UI animation and actally makes the sidebar a bit more responsive. Thank you :)
What?
I noticed that in the site editor, the
Layout
component re-renders too often causing almost everything to re-render. One of the reasons the layout component renders is when navigating in the site editor. This is normal because that component depends on the url... but Layout renders different parts of the UI and not all of them depend on the url. For instance theSiteHub
component is just a UI piece that remains untouched over time so it shouldn't re-render that often.This PR refactors the SiteHub component a little bit to prevent the re-rendering when navigating in the site editor. I wasn't able to measure a noticeable difference at the moment but I still think it's a good improvement without hurting the code base or anything (make it better instead).
Testing Instructions
Nothing should change it's just a code quality improvement.