feat(gatsby-image): Responsive Art Direction #26119

polarathene · 2020-07-30T04:15:49Z

Description

Presently, a render is triggered from an onload event calling handleImageLoaded() and fluid type aspectRatio or fixed type width & height state values for CSS are only updated to the new variant after it's loaded.

Placeholder lazy loading state is also not reset as the component isn't aware of when loading a new variant begins. If the image variant is cached, there is still a delay with the onload event until the image request has returned a response from the server; a noticeable delay on high latency connections - breaking layout (as the content/component has not been updated yet to fit the new breakpoint).

matchMedia() resolves that. We can then detect if the image has been cached internally and then trigger a render which might leverage browser cache detection, otherwise reset the lazy load placeholder behavior.

See individual commits for more logical grouping and further details related to those additions.

May need to rebase after the following PRs are merged:

Dependent on chore(gatsby-image): Add jest-matchmedia-mock to replace DIY mock #26118 to pass tests.
Implements a related conditional as this PR.

My "deprecated" comment for MediaQueryListListener(note, modern browsers alias this for now) is not quite correct, MDN encourages using the newer standard event mechanism, the older non-standard API is kept by browsers for backward compatibility. A newer version of Safari that's to be released this year will also support the newer API. MDN browser compatibility

Related Issues

Originally implemented in this PR, which has been split out into several smaller ones.

pvdz · 2020-07-30T10:42:55Z

Dependent PR has been merged, ready to rebase this.

Headsup that I probably won't get around to take a look until later today or tomorrow.

Additionally, I'd expect @wardpeet to also hope for e2e coverage for this PR. Especially if none of the tests need to change for that (but perhaps you're waiting to rebase before changing those).

polarathene · 2020-07-30T12:05:45Z

hope for e2e coverage for this PR.

I'd need some guidance on how to approach that.

I don't know if there is currently any high-latency network tests or any tests that track CSS transition or caching behaviour? They're generally assuming that if X and Y inputs are provided, Z output is expected, these kind of tests would be more nuanced..

If desirable, I can share some clips of what's being fixed, before/after of this PR under network throttled "Slow 3G"?

Especially if none of the tests need to change for that (but perhaps you're waiting to rebase before changing those).

The tests only broke and required the proper Jest matchMedia() mock due to not being able to run the new code using other matchMedia() API surface in Jest's JSDOM (which lacks any support for the API).

The existing Art Direction tests ensure the image source changes accordingly to meet it's media condition, but that's it.

wardpeet · 2020-09-15T15:58:39Z

packages/gatsby-image/src/index.js

+          if (mql.addEventListener) {
+            mql.addEventListener(`change`, this.handleVariantChange)
+          } else {
+            // Deprecated 'MediaQueryList' API, <Safari 14, IE, <Edge 16
+            mql.addListener(this.handleVariantChange)


We probably want to clean it up with componentWillUnmount. Besides that this looks great! Thank you!

I'm happy to take over as you've been waiting long on this feedback.

Presently, a render is triggered from `onload` event calling `handleImageLoaded()` and state such as fluid type `aspectRatio` or fixed type `width` & `height` values for CSS are only updated then. Placeholder lazy loading state is also not reset as the component isn't aware of when loading a new variant begins, if the variant is cached, there is still a delay in `onload` event until the image request has returned a response from the server, noticeable delay on high latency connections, breaking layout(as content/component has not updated to fit the new breakpoint as soon as it should). `matchMedia()` resolves that. We can then detect if the image has been cached internally and then trigger a render which might leverage browser cache detection, otherwise reset the lazy load placeholder behavior.

…andleImageVariant()`, we don't have to rely on `imgLoaded` state (with no change from `true`) in `handleImageLoaded()` to render an update. Instead we can ensure that it only sets `imgLoaded` to `true` as it's intended for, only when the state is actually `false`.

Unclear if this should be supported for image variants, but now enables supporting this feature when the image variant changes too, instead of just the first time(`componentDidMount()` or for Intersection Observer in `handleRef()`).

polarathene · 2020-09-17T06:03:32Z

Rebased onto master.

@wardpeet good catch there, sorry about missing that!

Handing this PR over to you to wrap up thanks.

wardpeet · 2020-09-17T06:15:48Z

@polarathene Thank you, I'm grateful that you're taking the time to finish it. Awesome work so far! 👍 👏

polarathene · 2020-09-17T08:27:16Z

I'm grateful that you're taking the time to finish it.

Sorry, we have a misunderstanding. You said earlier you'd be happy to take over the PR, and I thank you for that. I need to prioritize my time on other tasks that get me closer towards earning a living and landing my first job as a web developer.

I've only been handling the other related PR that doesn't seem like it'll be merged, as expecting users to spend time investigating gatsby-image internals and each DIY an Art Direction fix with a HOC isn't a pleasant experience to offload to the user (I should know, considering how much time I have sunk into resolving these issues :P ). Since I know gatsby-image well enough, I updated that PR to at least give other users a good reference that should work well.

wardpeet · 2020-09-17T09:30:23Z

OH yeah my bad "Handing this PR over to you to wrap up thanks." I'll take it over :)

I've only been handling the other related PR that doesn't seem like it'll be merged, as expecting users to spend time investigating gatsby-image internals and each DIY an Art Direction fix with a HOC isn't a pleasant experience to offload to the user (I should know, considering how much time I have sunk into resolving these issues :P ). Since I know gatsby-image well enough, I updated that PR to at least give other users a good reference that should work well.

I know, that's why we are in desperate need to create a new gatsby-image that properly tell the user what it can do and what you need to do yourself.

polarathene · 2020-09-17T09:48:49Z

I'll take it over :)

Thanks!

I know, that's why we are in desperate need to create a new gatsby-image

Wasn't that meant to be solved with the refactor to hooks? I had inquired about that to you in an issue/PR back around May-June, asking if it was being worked on publicly or if the core team had a private refactoring going on. Offered to collaborate on that, but never heard back.

I'm not currently in a position to have spare time for a hooks refactor anymore, but if my situation improves within the next 6 months, I might be able to give it a shot if there's still no progress. Originally I wanted to get these June PRs discussed with the team, iterated on and merged, then tackle the hooks refactor in advance for Gatsby consider when ready.

wardpeet · 2020-09-17T14:37:06Z

I have something to share in a few days :) Here is a sneak peek, I've only started on it a few weeks ago and mostly was an exploration on how to fix LCP web-vitals.

Of course through graphql you can just use a fragment and do {...data.whateveryouqueried}

    <React.Fragment>
      <div>Hello world!</div>
      <GatsbyImage
        width={500}
        height={600}
        placeholder={{
          fallback: fixedLong.base64,
        }}
        images={{
          fallback: {
            src: fixedLong.src,
            srcSet: fixedLong.srcSet,
          },
          sources: fixedImages,
        }}
        alt="hello"
      />
      <GatsbyImage
        {...data.file.childImageSharp.fluid}
        loading="eager"
        alt="hello"
      />

      <GatsbyImageHydrator>
        some extra stuff
        <div {...getWrapperProps()}>
          <Placeholder {...getPlaceholderProps()} />
          <MainImage {...getMainImageProps()} alt="zzz" />
        </div>
      </GatsbyImageHydrator>
    </React.Fragment>

Users expressed concern about the additional render triggered from this state update when unnecessary.

With changes to the related complimentary Art Direction PR, the `isBrowser` conditional can be removed and added to `isArtDirected` as a guard instead.

polarathene · 2020-09-21T01:55:23Z

Minor update to accommodate the request here to avoid isHydrated adding an unnecessary render for anyone not using Art Direction.

wardpeet · 2020-11-09T09:39:22Z

Closing, this is fixed in our new gatsby-plugin-image component. Thank you for your contribution! <3

polarathene requested a review from a team as a code owner July 30, 2020 04:15

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 30, 2020

This was referenced Jul 30, 2020

fix(gatsby-image): React hydration buster on image #24811

Closed

chore(gatsby-image): Add jest-matchmedia-mock to replace DIY mock #26118

Merged

polarathene requested a review from pvdz July 30, 2020 04:31

pvdz 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 Jul 30, 2020

polarathene force-pushed the feat/responsive-art-direction branch from 9b9edca to e31abde Compare July 30, 2020 11:55

polarathene mentioned this pull request Aug 14, 2020

Best strategy for checking whether (typeof window !== undefined) while using Hooks #26448

Closed

polarathene mentioned this pull request Sep 12, 2020

fix(gatsby-image): Don't assume DOM state is valid at hydration stage #26097

Merged

wardpeet reviewed Sep 15, 2020

View reviewed changes

polarathene added 3 commits September 17, 2020 17:47

polarathene force-pushed the feat/responsive-art-direction branch from e31abde to 058184f Compare September 17, 2020 05:54

polarathene added 2 commits September 21, 2020 13:52

fix: Only trigger a render with isHydrated when using Art Direction

be31cd7

Users expressed concern about the additional render triggered from this state update when unnecessary.

chore: Use isArtDirected in getCurrentSrcData()

93b24ab

With changes to the related complimentary Art Direction PR, the `isBrowser` conditional can be removed and added to `isArtDirected` as a guard instead.

polarathene mentioned this pull request Sep 30, 2020

MediaQueryList.prototype.addListener & removeListener are marked as deprecated microsoft/TypeScript#32210

Open

wardpeet closed this Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gatsby-image): Responsive Art Direction #26119

feat(gatsby-image): Responsive Art Direction #26119

polarathene commented Jul 30, 2020 •

edited

Loading

pvdz commented Jul 30, 2020

polarathene commented Jul 30, 2020 •

edited

Loading

wardpeet Sep 15, 2020

wardpeet Sep 15, 2020

polarathene commented Sep 17, 2020

wardpeet commented Sep 17, 2020

polarathene commented Sep 17, 2020

wardpeet commented Sep 17, 2020

polarathene commented Sep 17, 2020

wardpeet commented Sep 17, 2020 •

edited

Loading

polarathene commented Sep 21, 2020

wardpeet commented Nov 9, 2020

feat(gatsby-image): Responsive Art Direction #26119

feat(gatsby-image): Responsive Art Direction #26119

Conversation

polarathene commented Jul 30, 2020 • edited Loading

Description

Related Issues

pvdz commented Jul 30, 2020

polarathene commented Jul 30, 2020 • edited Loading

wardpeet Sep 15, 2020

Choose a reason for hiding this comment

wardpeet Sep 15, 2020

Choose a reason for hiding this comment

polarathene commented Sep 17, 2020

wardpeet commented Sep 17, 2020

polarathene commented Sep 17, 2020

wardpeet commented Sep 17, 2020

polarathene commented Sep 17, 2020

wardpeet commented Sep 17, 2020 • edited Loading

polarathene commented Sep 21, 2020

wardpeet commented Nov 9, 2020

polarathene commented Jul 30, 2020 •

edited

Loading

polarathene commented Jul 30, 2020 •

edited

Loading

wardpeet commented Sep 17, 2020 •

edited

Loading