-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
feat: make Markdown images lazy loaded #6598
Conversation
✔️ [V2] 🔨 Explore the source changes: 31a8d06 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61fd10d75df4260007f728ed 😎 Browse the preview: https://deploy-preview-6598--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6598--docusaurus-2.netlify.app/ |
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 👍
To me this looks fine to apply this by default, as content images are less important to load compared to other images (like layout images)
Later we'll introduce an official core <Img>
component in Docusaurus and it's a common practice to add some IntersectionObserver logic to make it load eagerly images that are close to entering the viewport (similar to links/idealImage)
Note users that want to disable lazy loading can still disable this thanks to a remark plugin. It's not very convenient/elegant but it's just erasing loading="lazy"
from the image jsx string.
I think we can ship this by default and see if anyone complains, to eventually revert or provide an option.
My only concern: do we want lazy loading for images we couldn't compute width/height? Not sure when this happens but it may lead to annoying layout shifts as the user scrolls down.
Any opinion @Josh-Cena ?
...s/docusaurus-mdx-loader/src/remark/transformImage/__tests__/__snapshots__/index.test.ts.snap
Outdated
Show resolved
Hide resolved
My opinions were given in that canny post. I'm also okay with this feature. My internet's wonky tonight, so I'll look over this during the weekend I hope |
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.
Let's do this! Later in #5999 we can try to make it configurable
Motivation
I would like to improve the performance of sites built with Docusaurus by making use of the native support in browsers for lazy loading images to reduce the number of HTTP requests made on page load.
I was able to do this using a rehype plugin which I wrote about here: https://blog.johnnyreilly.com/2022/02/02/lazy-loading-images-with-docusaurus
However, it occurred to me that all images being lazy loaded might not be a bad thing. Hence this pull request which adds the attribute
loading="lazy"
toimg
elements.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
You can see from the code that the snapshots have changed, such that images are lazy loaded due to the attribute
loading="lazy"
being added toimg
elements.Related PRs
Not a related PR, but this was also discussed here: https://docusaurus.io/feature-requests/p/lazy-loading-images-in-blog-posts-by-default and here: https://twitter.com/swyx/status/1488506746360401930
cc @sw-yx