-
-
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
[docs] Lazy load landing page images #17827
Conversation
Details of bundle changes.Comparing: 6b9ecbe...1dabd42
|
@@ -205,6 +205,9 @@ function HomeSteps(props) { | |||
className={classes.img} | |||
alt="themes" | |||
src={`/static/images/themes-${theme.palette.type}.jpg`} | |||
loading="eager" | |||
width={500} |
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 believe we override the dimension with CSS, do we need to specify something here?
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 browser needs to to calculate a proper placeholder and avoid layout shifts.
Resolved conflicts with #17830. |
e70a633
to
1dabd42
Compare
preview
Uses
loading="lazy"
for every image on the landing page except the themes which are eagerly loaded (since they are a conversion target).This does not have any effect on desktop viewports. At around 300-400 px height you get some images lazy loaded but only the logos of the companies that use our components. I guess this is fine for now since the benefit will only go up with more content on the landing page.
It might make sense to use this on other pages as well. Though I would not use it in any demo since it's very new and only supported on recent chrome releases. I want to avoid that people use those in their actual apps assuming it's either implemented by Material-UI or by our supported browsers.