-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[icons][docs] Virtualize icons svg #43939
Conversation
Netlify deploy previewhttps://deploy-preview-43939--material-ui.netlify.app/ Bundle size report |
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.
The UX is different when the CPU is overloaded, it's feels better to have the icon label readable, so this PR feels better IMHO.
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.
There is a clear performance win: https://www.webpagetest.org/video/compare.php?tests=240930_AiDcPC_94H,240930_BiDc37_94N.
The FCP is 2.5s faster:
TTI isn't much different though.
// Virtualize the icons to reduce page size and React rendering time. | ||
// Only render the icons after they become visible in the viewport. | ||
React.useEffect(() => { | ||
const margin = 200; |
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.
margin
to hoist?
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.
Kept it here to prevent it from being used for anything other than virtualization. Separation of concerns.
It looks good. My only question is, why not use |
We're not virtualizing in the same way here. As in, we're not rendering only a subset of the items and reserve empty space for the items that we don't render. Instead we render every item, only for the ones that are in the viewport, we render more content. |
Great, this is a clear step forward. We can check in 30 days how this helped with the core metrics #41126 (comment). Overall, our core web vitals are improving https://search.google.com/search-console/core-web-vitals?resource_id=sc-domain%3Amui.com, that's a good trend: We took a serious hit with https://web.dev/blog/inp-cwv-march-12. |
After seeing #43911, I realized I still had this experiment on a local branch.
This virtualizes the icons in a different way than #41330. It renders a placeholder for the svg when not in the viewport, it retains all other markup for SEO. This not only cuts rendering time of the list by two thirds, it also looks like it could drastically reduce page size.
Before:
https://deploy-preview-43937--material-ui.netlify.app/material-ui/material-icons/
After:
https://deploy-preview-43939--material-ui.netlify.app/material-ui/material-icons/
The first 50 icons are always rendered as they are likely visible in the viewport on page load.
Help with #41126