Skip to content
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

fix(gatsby-plugin-image): Correct IntersectionObserver handling #28309

Merged
merged 8 commits into from
Nov 26, 2020

Conversation

ascorbic
Copy link
Contributor

It turns out that the intersectionobserver never worked proeprly, but our tests didn't catch it because it works for the first image but not subsequent ones, and aside from Safari the images were all loading immediately when they were inside the intersection threshold. The problem was that we were using a shared IO instance (which is fine), but the IO's event listener was only calling the callback of the element that originally created it, and not any that were subsequently observed. This fixes it by using a WeakMap to map the image DOM element to its callback.

This also fixes the intersection thresholds, which were an arbitrary 150%. They now match the values used by Chrome native lazy loading, which vary according to the connection speed (where available).

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 26, 2020
wardpeet
wardpeet previously approved these changes Nov 26, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Could you add an e2e test so we don't have any regressions in the future?

@wardpeet wardpeet added topic: media Related to gatsby-plugin-image, or general image/media processing topics and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 26, 2020
@mfrachet
Copy link
Contributor

mfrachet commented Nov 26, 2020

@wardpeet this is exactly why we actually have our conversation about devtools and Cypress 😊 : I'm writing them

ascorbic and others added 2 commits November 26, 2020 14:43
@wardpeet
Copy link
Contributor

E2e test can be in a follow up, no need to block this PR for it

@ascorbic
Copy link
Contributor Author

ascorbic commented Nov 26, 2020

Marvin's a machine, and they're already in. I've just asked if he can move them to a different suite, but otherwise they look great to me

…-observer.js

Co-authored-by: Ward Peeters <ward@coding-tech.com>
wardpeet
wardpeet previously approved these changes Nov 26, 2020
@mfrachet
Copy link
Contributor

@ascorbic
Copy link
Contributor Author

@mfrachet It'll hit that if you're running gatsby develop, but these tests run gatsby build/serve, so they will have hasSSR set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: media Related to gatsby-plugin-image, or general image/media processing topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants