Opt-out of lazy-load: Add 'critical' flag to Img component #7083

tbrannam · 2018-08-06T22:29:36Z

Address #4297

Add critical attribute to opt-out of lazy-loading behavior
Picture tag to allow browser to select best matching file format (webp detection)
if fadeIn is true (default) - then the loaded image still will not appear until after the component is mounted

	    <Img fixed={images.file1.childImageSharp.fixed} />
	    <Img fixed={images.file1.childImageSharp.fixed} />
	    <Img fixed={images.file2.childImageSharp.fixed} critical fadeIn={false}/>
	    <Img fixed={images.file2.childImageSharp.fixed} critical/>

tbrannam · 2018-08-06T22:30:14Z

packages/gatsby-image/src/index.js

@@ -69,24 +69,6 @@ const listenToIntersections = (el, cb) => {
  listeners.push([el, cb])
 }

-let isWebpSupportedCache = null


Using picture tag makes this obsolete

tbrannam · 2018-08-06T22:32:00Z

packages/gatsby-image/src/index.js

@@ -141,7 +123,7 @@ class Image extends React.Component {

    // If this image has already been loaded before then we can assume it's
    // already in the browser cache so it's cheap to just show directly.
-    const seenBefore = inImageCache(props)
+    const seenBefore = !props.critical && inImageCache(props)


This may be a bit controversial - if a critical image is used in multiple places on a page, but has different fadeIn values - one of the critical image may not appear when the component is mounted.

tbrannam · 2018-08-06T22:32:39Z

packages/gatsby-image/src/index.js

@@ -168,10 +154,16 @@ class Image extends React.Component {
    this.handleRef = this.handleRef.bind(this)
  }

+  componentDidMount() {
+    if (this.props.critical) {
+      this.setState({imgLoaded: true});


onLoad can not be relied on to change the state for images that where loaded from the DOM

What did you change here? If you push individual commits, that's helpful to people reviewing your code.

BTW, did you see #4297 (comment)

Here I fixed up the lint error about spaces around braces.

KyleAMathews · 2018-08-06T22:33:26Z

Deploy preview for using-postcss-sass failed.

Built with commit fd48bb3

https://app.netlify.com/sites/using-postcss-sass/deploys/5b68dfb4e39e7c630cd35077

tbrannam · 2018-08-06T22:33:44Z

packages/gatsby-image/src/index.js

  handleRef(ref) {
    if (this.state.IOSupported && ref) {
      listenToIntersections(ref, () => {
-        this.setState({ isVisible: true, imgLoaded: false })
+        this.setState({ isVisible: true })


Not sure why this was set to false here. Reseting the state to false causes issues with critical images, and I could not find where non-critical images required this state change.

hmmm yeah, don't remember either 😅

tbrannam · 2018-08-06T22:35:14Z

packages/gatsby-image/src/index.js

+                  alt={alt}
+                  title={title}
+                  src={image.src}
+                  srcSet={image.srcSet}


Is there really a need to include srcSet, and sizes in the fallback? It appears that browsers that support these on img also implement picture - so it is added weight, without a purpose.

👍 sounds right to me.

gatsbybot · 2018-08-06T22:36:00Z

Deploy preview for using-drupal ready!

Built with commit fd48bb3

https://deploy-preview-7083--using-drupal.netlify.com

gatsbybot · 2018-08-06T22:36:27Z

Deploy preview for gatsbygram ready!

Built with commit fd48bb3

https://deploy-preview-7083--gatsbygram.netlify.com

KyleAMathews · 2018-08-06T22:37:02Z

Deploy preview for gatsbyjs failed.

Built with commit fd48bb3

https://app.netlify.com/sites/gatsbyjs/deploys/5b68dfb3e39e7c630cd3506b

tbrannam · 2018-08-07T16:58:56Z

packages/gatsby-image/src/index.js

-                }),
-              }}
-            />
+            {!this.props.critical && (


May need to walk this back - if fadeIn is true - and there is no scripting enabled - the loaded image will not appear

Is this still true, or was this fixed? I can't see which commits this came from 🙃

this has been revised like so:

gatsby/packages/gatsby-image/src/index.js

Line 323 in 198535f

{this.state.hasNoScript && (

pieh · 2018-08-17T11:28:59Z

packages/gatsby-image/src/index.js

+                  srcSet={image.srcSet}
+                  sizes={image.sizes}
+                  style={imageStyle}
+                  onLoad={() => {


should onLoad event handler be used in <source> as well? or maybe it would work using it in <picture>?

Img is a required tag in picture. It would seem to me that it would be redundant?

tbrannam · 2018-08-17T15:04:03Z

I have adjusted my local branch to support fadeIn for when just is disabled, but is has the side effect of having two instances of the picture loaded (the first has opacity:0 and the second is presented via the noscript block. I don't see a way around this. I am adjusting the noscript image to also use picture so that the same asset will be used and the asset will only be downloaded once.

All that should be left is verifying that onload on Gatsby-Image is called if the image is downloaded before the script bundle is loaded.

KyleAMathews · 2018-08-21T19:37:59Z

Deploy preview for using-contentful failed.

Built with commit 2c873b3

https://app.netlify.com/sites/using-contentful/deploys/5b7c68c5e39e7c6abe678f98

Implementation notes: * use Picture tag to allow browser to select best matching file format (webp detection) * if fadeIn is true (default) - then the loaded image still will not appear until after the component is mounted * the inImageCache should ignore 'critical' assets or images that have different fadeIn behaviors will not trigger an appropriate change in state (won't show loaded image) see example: ``` <Img fixed={images.file1.childImageSharp.fixed} /> <Img fixed={images.file1.childImageSharp.fixed} /> <Img fixed={images.file2.childImageSharp.fixed} critical fadeIn={false}/> <Img fixed={images.file2.childImageSharp.fixed} critical/> ```

Mimic behavior of lazy-images bypassing fadeIn when `seenBefore==true` Trigger load-complete script on componentDidMount if the image is loaded prior to the JS load complete Modify imagePlaceholder transitionDelay to reduce flicker present on critical images at tail end of fade (observed on Chrome 67.0) `noscript` `img` transitioned to use `picture` tag so that only one resource is downloaded in the event that JS is disabled when `critical==true + fadeIn==false`

Added webp src to mock data Updated Snapshot with <picture>

tbrannam · 2018-08-22T00:31:04Z

@KyleAMathews not sure about the ci failures. Looks like most prs are failing? Otherwise I think this PR is feature complete.

tbrannam · 2018-08-22T01:26:42Z

packages/gatsby-image/src/index.js

    this.state = {
      isVisible,
      imgLoaded,
      IOSupported,
+      fadeIn,
+      hasNoScript,
+      seenBefore,


stored to evaluate on mount to emulate the lazy behavior

tbrannam · 2018-08-22T01:27:50Z

packages/gatsby-image/src/index.js

    }

+    this.imageRef = React.createRef()


need a reference to the HTMLImageElement to evaluate the completed state - to detect if DOM finished loading image before the JS started

tbrannam · 2018-08-22T01:30:32Z

packages/gatsby-image/src/index.js

@@ -194,13 +214,14 @@ class Image extends React.Component {

    const imagePlaceholderStyle = {
      opacity: this.state.imgLoaded ? 0 : 1,
-      transitionDelay: `0.25s`,
+      transitionDelay: this.state.imgLoaded ? `0.5s` : `0.25s`,


when transitioning a critical image to opaque:1 - Chrome 67/68 flashes/flickers - not certain if there's a reason to synchronize the placeholder while fading in the real image.

tbrannam · 2018-08-22T01:31:46Z

packages/gatsby-image/src/index.js

      ...imgStyle,
      ...placeholderStyle,
    }

    const imageStyle = {
-      opacity: this.state.imgLoaded || this.props.fadeIn === false ? 1 : 0,
+      opacity: this.state.imgLoaded || this.state.fadeIn === false ? 1 : 0,


Fade in now copied from props at construction time. Can be mutated if a critical image has been seen before.

tbrannam · 2018-08-29T15:27:50Z

resolves #4297

DSchau · 2018-09-07T17:30:55Z

@tbrannam I plan to check this out this afternoon, so stay tuned :)

First thing I noticed is we're using the picture element now--which is cool--but that's not supported in IE 11 (per usual...). I think this will gracefully fallback to the image element (i.e. the picture tag will just be ignored) but it could be worth mentioning this in the gatsby-image documentation and then linking to appropriate polyfills. What do you think?

tbrannam · 2018-09-07T18:20:32Z

Yes - picture was built to fallback to img (img is a required field) I don't think a polyfill would be necessary. IE11 doesn't appear to support srcset at all either - so I think the behavior I think is unchanged with this PR. It's probably worth mentioning in the docs.

…

On Fri, Sep 7, 2018 at 1:31 PM Dustin Schau ***@***.***> wrote: @tbrannam <https://github.com/tbrannam> I plan to check this out this afternoon, so stay tuned :) First thing I noticed is we're using the picture element now--which is cool--but that's not supported in IE 11 (per usual...). I *think* this will gracefully fallback to the image element (i.e. the picture tag will just be ignored) but it could be worth mentioning this in the gatsby-image documentation and then linking to appropriate polyfills. What do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7083 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz3RduRUfaJRQoWg5ieu3QoHVKPCcRks5uYq1igaJpZM4VxM3Y> .

DSchau

I think this looks great. We can/should document the usage of picture in gatsby-image/README.md, so that people are at least aware of the change.

I validated the following:

No regressions to existing functionality with examples/image-processing project
critical attribute set to true and fadeIn of false seem to work like I'd expect

@pieh want to chime in here, or are we good to go on merging this?

Update transition style for no fade from `<empty>` to `none`

Added text mentioning the use of `<picture>`

DSchau · 2018-09-11T16:42:01Z

packages/gatsby-image/README.md

@@ -277,3 +277,6 @@ prop. e.g. `<Img fluid={fluid} />`
 - Images marked as `critical` will start loading immediately as the DOM is 
  parsed, but unless `fadeIn` is set to `false`, the transition from placeholder
  to final image will not occur until after the component is mounted.
+- Gatsby-Image now is backed by newer `<picture>` tag. This newer standard allows for


👍 Thanks for this!

gatsbot · 2018-09-11T16:49:09Z

Holy buckets, @tbrannam — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@tbrannam

In #7083 the css `transition` was moved [from Img](https://github.com/gatsbyjs/gatsby/blob/bcf3af57f3b6438424b712ce1785a6a423b937da/packages/gatsby-image/src/index.js#L117) to [imageStyle](https://github.com/gatsbyjs/gatsby/blob/5ffb1307c17bef802026d5992e1ad000ff654f11/packages/gatsby-image/src/index.js#L224), leaving [imagePlaceholderStyle with a transitionDelay](https://github.com/gatsbyjs/gatsby/blob/5ffb1307c17bef802026d5992e1ad000ff654f11/packages/gatsby-image/src/index.js#L217), but without an actual transition. Although I don't follow everything that changed with that commit, I think that wasn't on purpose. @tbrannam can you confirm?

@tbrannam

In gatsbyjs#7083 the css `transition` was moved [from Img](https://github.com/gatsbyjs/gatsby/blob/bcf3af57f3b6438424b712ce1785a6a423b937da/packages/gatsby-image/src/index.js#L117) to [imageStyle](https://github.com/gatsbyjs/gatsby/blob/5ffb1307c17bef802026d5992e1ad000ff654f11/packages/gatsby-image/src/index.js#L224), leaving [imagePlaceholderStyle with a transitionDelay](https://github.com/gatsbyjs/gatsby/blob/5ffb1307c17bef802026d5992e1ad000ff654f11/packages/gatsby-image/src/index.js#L217), but without an actual transition. Although I don't follow everything that changed with that commit, I think that wasn't on purpose. @tbrannam can you confirm?

tbrannam commented Aug 6, 2018

View reviewed changes

tbrannam commented Aug 7, 2018

View reviewed changes

pieh reviewed Aug 17, 2018

View reviewed changes

Todd Brannam added 3 commits August 21, 2018 15:41

Update Unit Tests

198535f

Added webp src to mock data Updated Snapshot with <picture>

tbrannam commented Aug 22, 2018

View reviewed changes

tbrannam mentioned this pull request Aug 29, 2018

Allow optional disabling of lazy loading images in SSR to improve performance #4297

Closed

DSchau self-assigned this Sep 7, 2018

DSchau approved these changes Sep 7, 2018

View reviewed changes

tbrannam added 2 commits September 7, 2018 21:45

Update index.js

68a6da9

Update transition style for no fade from `<empty>` to `none`

Update README.md

7d3f389

Added text mentioning the use of `<picture>`

DSchau reviewed Sep 11, 2018

View reviewed changes

DSchau merged commit 5ffb130 into gatsbyjs:master Sep 11, 2018

CanRau mentioned this pull request Sep 18, 2018

maybe add info about breaking changes in gatsby-image #8254

Closed

stefanprobst mentioned this pull request Sep 20, 2018

[gatsby-image] Restore placeholder image transition #8350

Merged

tbrannam deleted the topics/tcb-image-critial branch November 4, 2018 15:47

georges-gomes mentioned this pull request Feb 21, 2023

Warning for images being already lazy-loaded divriots/jampack#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opt-out of lazy-load: Add 'critical' flag to Img component #7083

Opt-out of lazy-load: Add 'critical' flag to Img component #7083

tbrannam commented Aug 6, 2018 •

edited

Loading

tbrannam Aug 6, 2018

tbrannam Aug 6, 2018 •

edited

Loading

tbrannam Aug 6, 2018

KyleAMathews Aug 7, 2018

tbrannam Aug 7, 2018

KyleAMathews commented Aug 6, 2018 •

edited

Loading

tbrannam Aug 6, 2018

KyleAMathews Aug 7, 2018

tbrannam Aug 6, 2018

KyleAMathews Aug 7, 2018

gatsbybot commented Aug 6, 2018 •

edited

Loading

gatsbybot commented Aug 6, 2018 •

edited

Loading

KyleAMathews commented Aug 6, 2018 •

edited

Loading

tbrannam Aug 7, 2018

DSchau Sep 7, 2018

tbrannam Sep 8, 2018

pieh Aug 17, 2018

tbrannam Aug 17, 2018

tbrannam commented Aug 17, 2018

KyleAMathews commented Aug 21, 2018

tbrannam commented Aug 22, 2018 •

edited

Loading

tbrannam Aug 22, 2018

tbrannam Aug 22, 2018

tbrannam Aug 22, 2018

tbrannam Aug 22, 2018

tbrannam commented Aug 29, 2018

DSchau commented Sep 7, 2018

tbrannam commented Sep 7, 2018 via email

DSchau left a comment •

edited

Loading

DSchau Sep 11, 2018

gatsbot bot commented Sep 11, 2018

Opt-out of lazy-load: Add 'critical' flag to Img component #7083

Opt-out of lazy-load: Add 'critical' flag to Img component #7083

Conversation

tbrannam commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

tbrannam Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleAMathews commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatsbybot commented Aug 6, 2018 • edited Loading

gatsbybot commented Aug 6, 2018 • edited Loading

KyleAMathews commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrannam commented Aug 17, 2018

KyleAMathews commented Aug 21, 2018

tbrannam commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrannam commented Aug 29, 2018

DSchau commented Sep 7, 2018

tbrannam commented Sep 7, 2018 via email

DSchau left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatsbot bot commented Sep 11, 2018

tbrannam commented Aug 6, 2018 •

edited

Loading

tbrannam Aug 6, 2018 •

edited

Loading

KyleAMathews commented Aug 6, 2018 •

edited

Loading

gatsbybot commented Aug 6, 2018 •

edited

Loading

gatsbybot commented Aug 6, 2018 •

edited

Loading

KyleAMathews commented Aug 6, 2018 •

edited

Loading

tbrannam commented Aug 22, 2018 •

edited

Loading

DSchau left a comment •

edited

Loading