-
Notifications
You must be signed in to change notification settings - Fork 10.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
Add support for native lazy loading to gatsby-image #13201
Comments
Excited to see the loading attribute being considered. Initially, those mappings for lazy vs eager LGTM. |
I would like to have a go at this, it would be my first contribution though so I'm unsure what the procedure is in taking an issue? |
Hey @andrewfairlie Thank you for offering to take this up! We really appreciate it and this would be an awesome first contribution to Gatsby. Would you like to pair on this sometime tomorrow and get this in? |
@siddharthkp 👍my schedule's a little wonky this week with a fair bit of travel. If it's cool with you I'll fork this and attempt when I have an hour or two and if I don't make much progress could we then arrange a pair session to get me on the right lines? Really appreciate the offer of pairing, very cool! 👍 |
@andrewfairlie we want to get this in pretty quickly so if you're not available this week, @sidharthachatterjee might just take it up directly. There's plenty of other issues tagged with
help wanted
We also offer pair programming sessions you can sign up for https://www.gatsbyjs.org/contributing/pair-programming/ |
That makes sense to me. I'll keep an eye on this and see how the problem was solved and will take another issue when I have the time. This will be a great feature 🤘 |
I can take it on since I've contributed a bit to It just involves adding the |
Yup! Though we'd reuse the You want to find a time to pair with @siddharthkp? |
I've not done pair programming before, especially remotely. I doubt my internet connection here is good enough for screen sharing or video chat, audio only probably isn't good either as the environment is often noisy(out of my control). Or was it more to do with some real-time sync with text editors and some chat (slack, discord, etc)? I don't know if it'll be all that useful to arrange if the work to be done is minimal. Sounds rather straight-forward with just a few lines to PR? I'm in NZ, it's 10.30PM atm, I've still got some other tasks to handle but should be free tomorrow to tackle this, if I get stuck I'll reach out :) I can see above though that you seem to want to get this out quickly, so by all means if you'd rather assign it to @siddharthkp to get merged ASAP, go for it! Otherwise, tomorrow I'll assign myself to this if it's unclaimed. |
/s/@siddharthkp/@sidharthachatterjee thx bye 😋 |
I missed these. Sorry @polarathene! 😞 Want to help review the PR at #13217? (@siddharthkp What's up? 😂) |
Should we support IE11? |
@Raincal that behaves a bit differently. It lowers loading priority but doesn't enable actual lazy loading. I guess it could be added easily enough, Is that beneficial? Since future versions of IE/Edge are adopting Blink, and the current browser usage for that attribute being low (~4.5% Global, ~2.5% China, ~0.25% India), it's probably best suited to the user adding the attribute manually when it can make a meaningful difference. |
Hiya! This issue has gone quiet. Spooky quiet. 👻 We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open! Thanks for being a part of the Gatsby community! 💪💜 |
Published in |
It seems like because of Chrome native lazy loading thresholds custom gatsby-image lazy loading behavior is lost. In Firefox images are loaded as soon as they are in viewport but in Chrome hadrcoded thresholds are used and there is no way to neither customize native Chrome threshold nor refuse it and just use gatsby-image custom logic, is it right? |
@alinenko yes that's correct. In a browser that supports native lazy loading, or does not have IntersectionObserver support, the visibility of the full image to load will be enabled(which will allow the browser to apply it's lazy/eager loading logic). The code that affects that is in the const isVisible =
this.isCritical ||
(isBrowser && (hasNativeLazyLoadSupport || !this.useIOSupport))
If you have a case for needing to control the trigger distance, perhaps look up the previous issue or PR about this, or create a new issue. You'll probably have to wait until |
@polarathene thank you for your response! Seems like the only way to handle this now is downgrading to pre-2.1.0. Works fine for Chrome but brings an issue to IE (even with polyfill usage) :( |
@alinenko if there is breakage and no existing issue for it, perhaps you should open a new issue about that regarding IE. More likely to be reviewed by others than this closed issue. You will want to provide more details about what version of IE and a small example to reproduce it so it can be investigated and confirmed. |
https://twitter.com/addyosmani/status/1114777583302799360
We'll want to feature detect this using the code in Addy's blog post:
If lazy-loading exists, then we'll want to use it, if not, we'll continue to use our IntersectionObserver setup.
For images using the
critical
prop, we should useloading="eager"
and for other images, we should useloading="lazy"
— this should replicate our existing semantics.The text was updated successfully, but these errors were encountered: