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

feat(gatsby-image): Add support for native lazy loading #13217

Merged
merged 15 commits into from May 16, 2019

Conversation

sidharthachatterjee
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee commented Apr 8, 2019

So... Chrome just added support for native lazy loading for images and iframes (https://twitter.com/addyosmani/status/1114777583302799360)

This PR adds support in gatsby-image so that we use native lazy loading when available and fall back to our IntersectionObserver code when not.

Test

yarn add gatsby-image@loading-attribute

Notes

I've exposed the loading attribute as a prop but just realised that we'll want to set that based on our critical prop as @KyleAMathews noted in #13201

Another thing to note is that we always set this for the no script Img tag (rendered during SSR and no js) since it's a progressive enhancement and setting it does not break anything in browsers that are yet to support it

Todo

  • Add some snapshot tests
  • Update README and documentation for gatsby-image
  • Blog post
  • Deprecate critical prop and set loading based on it
  • Update using-gatsby examples

Related

packages/gatsby-image/src/index.js Show resolved Hide resolved
`loading` in HTMLImageElement.prototype
) {
// Setting isVisible to true to short circuit our IO code and let the browser do its magic
isVisible = true
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe to make it clear this is now representing two different states it could be changed to isVisibleOrNativeLazyLoadingSupported — verbose is effective :-D

Copy link
Contributor

Choose a reason for hiding this comment

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

@KyleAMathews isn't the isVisible state here linked to the image components visibility state for it's picture element? I suggested a refactor here, but perhaps that's the wrong approach.

Copy link
Contributor

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

I see you got onto while I was asleep :P All good, I think you did a better job than if I had attempted it!

Provided a review as requested :)

packages/gatsby-image/src/index.js Show resolved Hide resolved
packages/gatsby-image/src/index.js Show resolved Hide resolved
packages/gatsby-image/src/index.js Show resolved Hide resolved
} = props

const loadingAttribute = {
...(nativeLazyLoadSupported && loading),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is nativeLazyLoadSupported boolean actually useful here?

This doesn't seem like the right approach for handling this single attribute. You're defining a const obj with a potential single prop that if the boolean prop is true, adopts the loading var as a key/prop to it's contained string value?

The destructuring approach here and below for this seems to harm readability. Why not instead go with:

loading={loading || `auto`}
// or conditionally, if the prop isn't specified, the attribute won't be assigned
loading={loading}

If the browser supports this attribute, auto is applied by default implicitly, there isn't a need to check for native support to assign it, makes no difference.

@DSchau DSchau added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Apr 9, 2019
@DSchau DSchau added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Apr 17, 2019
@DSchau
Copy link
Contributor

DSchau commented Apr 23, 2019

Fixes #13201

@polarathene
Copy link
Contributor

Any feedback to what I gave? If there's nothing wrong with it, I could perhaps make the changes as it seems @sidharthachatterjee is busy/away for a couple weeks now?

If so, how do I contribute to this PR? Or do I need to fork it and send another PR in? Or does @sidharthachatterjee still want to finish this PR?

@sidharthachatterjee sidharthachatterjee changed the title [WIP] feat(gatsby-image): Add support for native lazy loading feat(gatsby-image): Add support for native lazy loading May 16, 2019
@sidharthachatterjee sidharthachatterjee removed status: WIP status: awaiting author response Additional information has been requested from the author labels May 16, 2019
KyleAMathews
KyleAMathews previously approved these changes May 16, 2019
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Awesome work @sidharthachatterjee!

@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 16, 2019
@wardpeet wardpeet removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 16, 2019
@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 16, 2019
pieh
pieh previously approved these changes May 16, 2019
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM! (deju vu)

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

:shipit:

@KyleAMathews KyleAMathews merged commit 3c0eb1e into master May 16, 2019
@KyleAMathews KyleAMathews deleted the feat/add-native-lazy-loading-support branch May 16, 2019 19:19
@sidharthachatterjee
Copy link
Contributor Author

@pieh You mean a glitch in the Matrix

@sidharthachatterjee
Copy link
Contributor Author

Published in gatsby-image@2.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for native lazy loading to gatsby-image
6 participants