feat(gatsby-image): Add art direction

[brimtown]

Description

This PR adds art direction capabilities to gatsby-image.

See issue #13164 for additional background and discussion.

Decisions Made

1. Introduce new fixedImages and fluidImages props, instead of overloading the existing fixed and fluid
2. Only generate additional <sources> if there is more than one variant. This keeps the snapshots for the existing tests the same, except for the addition for a single <picture> tag for base64 / tracedSVG placeholders.
3. Use the first image's src as the key for the image cache, instead of creating new entries in the cache per source

I'm open to revisiting any of these, as each one is a tradeoff.

Related Issues

Fixes #13164, #4491, #7313

[brimtown]

I've done some manual QA on this by plugging it into a staging build of shopflamingo.com and testing across Chrome and Safari. Is there a better way to set up some shareable test links for this?

Also, would this PR be the place to add additions to the docs / the demo site?

[polarathene]

Phew, that took a while! 😛 I think I've covered everything pretty well.

Also, would this PR be the place to add additions to the docs / the demo site?

I'd prefer that as a separate PR after this one personally. It could reference this merge commit / PR, but should otherwise be kept separate due to size of this one.

Is there a better way to set up some shareable test links for this?

I'm sure there is a better way but in the past I've copied the package from the fork into it's own repo and referenced it in package.json, then hosted on Netlify.

[polarathene]

Note that in the fixed section, there is code you've not handled which references image. divStyle, bgColor and the Img tag(although I can't recall the impact of width/height attributes defined on an img element, have you confirmed variants with different width/height as fixed elements render correctly?

Adjusting the width/height attributes on the fixed image in this demo doesn't appear to have any effect, so I guess it's just the other two that should be taken into consideration?

fluid section also references the images aspectRatio for the paddingBottom style. That's a case which may have gone missed?

[polarathene]

Review Summary:

TL;DR: version, links to parts of the review for further information.

Questions / Concerns

• Potential breakage concern on update for users from the introduction of wrapping img elements in pictures.
• The placeholder generate..() methods are noops for instances not providing variants, while the noscript and image differ by handling non-variant cases, I don't like the inconsistency there, nor with handling of props/attributes.
• Are there any concerns with placeholder styles due to the picture element wrapping them? Should they stay on img or be moved to picture?
• Both fluid and fixed conditionals in the render have logic depending on image props that aren't supporting image variants with different widths/heights/aspectRatios. Discussed here
• Current implementation is creating both sources for the first image as well, but the non-webp source was removed in a prior commit due to Safari bug, will this introduce a regression? Discussed here

Change suggestions

• Use the generate sources methods to handle the variants only instead of catering to all sources which requires special handling affecting readability and consistency/predictability of how the group of methods behave. Add a clear && guard in the JSX so it's clearly separated as logic for handling multiple sources(helpful for when we refactor and split this file up).
• Drop the copy/override behaviour of convertProps() that assigns first fluidImages/fixedImages element to fluid/fixed.
• Refactor generateNoscriptSources() to one of the alternatives suggested in review here (also adjust for additional concerns raised on that link).
• Refactor the start of noscriptImg() as suggested in review here.
• Refactor generateImageSources() to one the suggestion in review here.
• Refactor the rendering for the main image (picture) element as suggested in review here, note comment to remove a guard.
• Refactor the start of fluid and fixed render conditionals, to have a base image var like we currently do, and variants as array(with either 1st element included or removed, based on other refactoring), as discussed in review here.

Misc

Not required for the PR to be approved, but related for future work:

• May want to include type attribute for all sources? Discussed here.

[polarathene]

Reviewing the changes branch commits 6282c75:Respond to code review and fc12f62:Use const in tests as requested for feedback by @brimtown .

packages/gatsby-image/src/index.js - Line #19:

// Find the source of an image to use as a key in the image cache.
// Use `fixed` or `fluid` if specified, or the first image in
// either `fixedImages` or `fluidImages`
const getImageSrc = props =>
  props.fluid
    ? props.fluid.src
    : props.fixed
    ? props.fixed.src
    : props.fluidImages
    ? props.fluidImages[0].src
    : props.fixedImages[0].src

Refactoring the ternary logic from the two locations into a function is good, but this nested ternary approach, not so much.

Would it be simpler to read if you instead destructure the props and use the || operator for selection? Same outcome I believe:

const getImageData = ({fluid, fixed, fluidImages, fixedImages}) => (
  fluid || fixed || fluidImages[0] || fixedImages[0]
)

const src = getImageData(convertedProps).src

or

const getImageSrc = ({fluid, fixed, fluidImages, fixedImages}) => {
  const data = fluid || fixed || fluidImages[0] || fixedImages[0]
  return data.src
}

const src = getImageSrc(convertedProps)

packages/gatsby-image/src/index.js - Line #155:

  let initialSources = ``
  if (props.srcSetWebp) {
    initialSources += generateNoscriptSource(props, true)
  }
  if (props.media) {
    initialSources += generateNoscriptSource(props)
  }

   const variantSources =
    props.imageVariants.length > 0
      ? generateNoscriptSources(props.imageVariants)
      : ``

   return `<picture>${initialSources}${variantSources}<img ${width}${height}${sizes}${srcSet}${src}${alt}${title}${crossOrigin}style="position:absolute;top:0;left:0;opacity:1;width:100%;height:100%;object-fit:cover;object-position:center"/></picture>`

Any reason for splitting sources into initialSources and variantSources here? My concerns earlier for keeping a distinction between this features additions were more related to what you were doing with your generateSources() methods. It'd be fine to be consistent here with another if conditional that appends to the same sources string. The feature would still have a clear separation.

packages/gatsby-image/src/index.js - Line #362:

// First image data is supplied to `image`, if an array the rest will go to `variants`
const [image, ...imageVariants] = fluidImages ? fluidImages : [fluid]

Slight mistake here as this was due to taking one of my suggestions. variants should be imageVariants in the comment. You may also want to add the comment for the same line in the fixed logic.

---

Other than that, all change requests are resolved. These concerns are still valid:

• fixed and fluid image conditional blocks both still have properties that don't adapt accordingly
• Any regression from this PR that would affect Safari again?

There is another issue regarding the cache key, but as it also affects the current gatsby-image code, it's not a blocker for accepting this PR, I'll raise that as a separate issue.

[brimtown]

Sorry @polarathene , I had missed that you had reviewed this before I integrated those changes into this branch! Yes, those sections you called out were the places I felt like had regressed in the latest part. Will update those further 😄.

For the other two remaining issues:

Check for Safari Regressions

I imagine the Safari validation will be fairly straightforward, and I will do that after this latest round of updates.

Check how properties adapt in different circumstances

I've been wondering how best to validate some of this. I had planned to make a sort of Gatsby Image Playground that tries out several of these options (both fixed and fluid, with and without critical, with the same and different aspect ratios / heights & widths) and using that as a test bed for some of the behavior here.

I think I need to explore some of those combinations a bit more, and figure out which places we may need to handle, which places we'd want to document, and if there are any combinations that wouldn't be supported. At least for the critical case, we wouldn't even know which image was currently selected (since the browser is determining that itself), so there may be some combinations that wouldn't work exactly the same way as in the non-art-directed version.

[KyleAMathews]

@brimtown would love to pair with you sometime on this! This is a really exciting PR!

[polarathene]

Is the length check necessary? It'll likely fail iirc checking the length property if the prop itself is undefined? Or is this because of convertProps() that it's safe to always assume an array value present?

I would add that we have both inImageCache() and activateCacheForImage() both calling this method after first doing:

const convertedProps = convertProps(props)

We might as well just move that duplication into this method and drop the need to pass in props?

Suggested change

	const data = (fluid.length && fluid[0]) || (fixed.length && fixed[0])
	const convertedProps = convertProps(props)
	const data = (fluid.length && fluid[0]) || (fixed.length && fixed[0])

EDIT: I see that your convertProps() method returns an array regardless, so if it's empty a 0 falsey value for fluid will select fixed instead. My bad :)

[polarathene]

Review done. Some questions for @wardpeet regarding some commits he made, along with some suggestions.

What is with the Danger failure? Was it trying to merge on green?

"triggeredByUsername": "wardpeet"

[wardpeet]

No clue about dangerjs. It happend when I pushed. We can just ignore it

[wardpeet]

I think I have resolved all review comments.

[polarathene]

No need to make the changes in these two comments, I can do them in my next PR.

LGTM, thanks @wardpeet for helping get this PR polished and ready for merging :) Big thanks to @brimtown for contributing this feature! 😃

[wardpeet]

Let's get this merged! Thank you for the reviews & hard work on this one!

[gatsbot]

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

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

1. 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. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. 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!

[antoinegrelard]

Hi @brimtown , thanks for this !

I just upgraded the package to get access to this and I find myself stuck with a rather annoying issue.

From my understanding, Art direction means we can display different images at different breakpoints, which means we can have different aspect ratios through viewport sizes. The issue I think is the following :

Given this sources array :

const fakeImage = {
  aspectRatio: 0.75,
  src: "https://via.placeholder.com/1440x1920",
  srcSet:
    "https://via.placeholder.com/300x400 300w, https://via.placeholder.com/600x800 600w, https://via.placeholder.com/900x1200 900w, https://via.placeholder.com/1440x1920 1440w",
  sizes: "(max-width: 1440px) 100vw, 1440px",
};

const fakeImageBig = {
  aspectRatio: 1.7777777778,
  src: "https://via.placeholder.com/1600x900",
  srcSet:
    "https://via.placeholder.com/300x169 300w, https://via.placeholder.com/600x338 600w, https://via.placeholder.com/1200x675 1200w, https://via.placeholder.com/1600x900 1600w",
  sizes: "(max-width: 1600px) 100vw, 1600px",
};

const sources = [
  fakeImage,
  {
    ...fakeImageBig,
    media: `(min-width: 768px)`,
  },
];

I would expect the aspect ratio to be at 0.75 below 768px, and be updated to 1.77778 over, but currently it's just taking the image.aspectRatio, which is fluid[0], therefore not updating the aspect ratio.

Am I just not doing the right thing ?

Thanks!

[brimtown]

Hi @antoinegrelard ! Yes, this is a current limitation with the code as is. Since gatsby-image uses style tags that don't support media queries, we de-scoped it to get this merged.

Regardless, I believe @polarathene had some ideas how to handle from Gatsby itself. I will open the issue on this and tag you in it, and it should be fixable 👍 .

In the meantime, we've worked around this in our own app code with a snippet like this:

// These styles fix a problem in `gatsby-image` where using
// art direction with images that have different aspect ratios
// defaults to using the aspect ratio of the first image passed
// to `fluidSources`. This is hardcoded to work for two images,
// but could be generalized to work for more. Ideally this
// will be fixed upstream in the future.
const ResponsiveImage = styled(GatsbyImage)`
  ${({ sources }) => `
    & > div:first-child {
      padding-bottom: ${100 / sources[0].aspectRatio}% !important;

      ${mediaQueries.DESKTOP
        padding-bottom: ${100 / sources[1].aspectRatio}% !important;
      `}
    }
  `}
`;

If you know how many variations you'll need, hardcoding it with something like this works fairly well. You could also map over sources and generate these more programmatically, which Gatsby will need to do to solve this directly.

Glad you're using the feature! I think once this gets fixed, we can share it more broadly in a blog post and improve some of the documentation here.

EDIT: I'd also be curious to know from your example if leaving off the media key in the first image causes both images to be downloaded (especially in Safari, which generally is less smart than Chrome on <picture>). I see that that's how the documentation has been written, but I'm not sure if that was specifically tested.