Skip to content

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

Merged
KyleAMathews merged 15 commits intomasterfrom
feat/add-native-lazy-loading-support
May 16, 2019
Merged

feat(gatsby-image): Add support for native lazy loading#13217
KyleAMathews merged 15 commits intomasterfrom
feat/add-native-lazy-loading-support

Conversation

@sidharthachatterjee
Copy link
Copy Markdown
Contributor

@sidharthachatterjee sidharthachatterjee commented Apr 8, 2019 •
edited
Loading

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

@sidharthachatterjee sidharthachatterjee mentioned this pull request Apr 8, 2019
Closed
KyleAMathews
KyleAMathews reviewed Apr 9, 2019
View reviewed changes
Comment thread packages/gatsby-image/src/index.js
Comment thread packages/gatsby-image/src/index.js
`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
Copy Markdown
Contributor

@KyleAMathews KyleAMathews Apr 9, 2019

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
Copy Markdown
Contributor

@polarathene polarathene Apr 9, 2019

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.

polarathene
polarathene reviewed Apr 9, 2019
View reviewed changes
Copy link
Copy Markdown
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 :)

Comment thread packages/gatsby-image/src/index.js
Comment thread packages/gatsby-image/src/index.js
Comment thread packages/gatsby-image/src/index.js
Comment thread packages/gatsby-image/src/index.js Outdated
} = props

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

@polarathene polarathene Apr 9, 2019

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
Copy Markdown
Contributor

DSchau commented Apr 23, 2019

Fixes #13201

@polarathene
Copy link
Copy Markdown
Contributor

polarathene commented Apr 23, 2019

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?

@KyleAMathews KyleAMathews mentioned this pull request May 2, 2019
Closed
@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
DSchau
DSchau reviewed May 16, 2019
View reviewed changes
Comment thread packages/gatsby-image/src/index.js Outdated
DSchau
DSchau reviewed May 16, 2019
View reviewed changes
Comment thread packages/gatsby-image/src/index.js Outdated
KyleAMathews
KyleAMathews previously approved these changes May 16, 2019
View reviewed changes
Copy link
Copy Markdown
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 reviewed May 16, 2019
View reviewed changes
Comment thread packages/gatsby-image/README.md Outdated
@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
View reviewed changes
Copy link
Copy Markdown
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!

pieh
pieh approved these changes May 16, 2019
View reviewed changes
Copy link
Copy Markdown
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)

KyleAMathews
KyleAMathews approved these changes May 16, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

sidharthachatterjee commented May 16, 2019

@pieh You mean a glitch in the Matrix

@sidharthachatterjee
Copy link
Copy Markdown
Contributor Author

sidharthachatterjee commented May 16, 2019

Published in gatsby-image@2.1.0

@polarathene polarathene mentioned this pull request May 19, 2019
Merged
raffishquartan pushed a commit to raffishquartan/gatsby that referenced this pull request Apr 28, 2026
@sidharthachatterjee @KyleAMathews
feat(gatsby-image): Add support for native lazy loading (gatsbyjs#13217)
a343b22 
* Add support for new native lazy loading to gatsby-image

* Add loading prop to typings

* Fix feature check

* Fix optional prop

* Update snapshots

* Deprecate critical and map its value to loading

* Document new loading attribute

* Update comment

* Apply suggestions from code review

Co-Authored-By: Dustin Schau <DSchau@users.noreply.github.com>

* chore: format

* Do not show deprecation message in production

* Clean up markdown table

* Clean up markdown table again

* Fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pieh pieh pieh approved these changes

+4 more reviewers

@wardpeet wardpeet wardpeet left review comments

@DSchau DSchau DSchau left review comments

@polarathene polarathene polarathene left review comments

@KyleAMathews KyleAMathews KyleAMathews approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sidharthachatterjee sidharthachatterjee

Labels

bot: merge on green Gatsbot will merge these PRs automatically when all tests passes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for native lazy loading to gatsby-image

6 participants

@sidharthachatterjee @DSchau @polarathene @KyleAMathews @pieh @wardpeet