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

[gatsby-image] Add art direction #13164

Closed
brimtown opened this issue Apr 5, 2019 · 17 comments · Fixed by #13395
Closed

[gatsby-image] Add art direction #13164

brimtown opened this issue Apr 5, 2019 · 17 comments · Fixed by #13395
Labels
stale? Issue that may be closed soon due to the original author not responding any more. type: bug An issue or pull request relating to a bug in Gatsby

Comments

@brimtown
Copy link
Contributor

brimtown commented Apr 5, 2019

Summary

Art direction refers to serving different source images at different breakpoints, in order to show users a different crop or image depending on their screen size.

CSS display: none

While art direction can be approximated from CSS using media queries and display: none, browsers (at least Chrome and Safari) will still load an image that has display: none. This is suboptimal from a performance and user-experience perspective.

Art Direction Loading Example: https://codesandbox.io/s/lp6746pkw9

HTML <picture> and <source>

A better way to accomplish art direction is by using a <picture> tag containing a <source> element per crop, with an accompanying media attribute. This allows the browser to choose the correct source to display depending on the matching media string.

With a few modifications, gatsby-image can support art direction for an arbitrary number of image crops.

High-Level Changes Needed

  1. Add an optional media key to both FluidObject and FixedObject, to pass in a media query string that corresponds to a specific image for the browser to match on.
  2. Allow gatsby-image to take Array<FluidObject> and Array<FixedObject>, to generate a <source> per image crop.

Basic example

Given a setup like this:

const MOBILE_QUERY = `(max-width: 767px)`;
const DESKTOP_QUERY = `(min-width: 768px)`;

// images here can be either fluid or fixed
const sources = [
  {
    ...mobileImage.fluid,
    media: MOBILE_QUERY,
  },
  {
    ...desktopImage.fluid,
    media: DESKTOP_QUERY,
  },
];

Option 1: Introduce new fluidSources / fixedSources props

<Image
  alt={mobileImage.description}
  fluidSources={sources}
/>

Having two new props to support this leads to some potential footguns (what if a user passes both fluid and fluidSources?), and additional branching logic in gatsby-image.

Option 2: Augment existing fluid / fixed props to take an array of images

<Image
  alt={mobileImage.description}
  fluid={sources}
/>

This option removes the need to introduce any new props, and would allow the internals of gatsby-image to be refactored to always operate in terms of an array of images. Passing a single image here would just be treated as an array of length 1.

Option 3 (Not Considered): New multi-image Component

If this were a separate component, it would want to have many of the same features as gatsby-image does (image cache, lazy loading, base64 / tracedSVG, etc.). This would seem to require a much larger refactor of gatsby-image to make these features shareable, and would increase the barrier to adopting art direction incrementally.

Motivation

This feature has been previously requested (#4491, #7313), with some indication of support from @KyleAMathews .

By having gatsby-image handle this directly, users won't need to choose between using all of gatsby-image's many benefits (lazy loading, critical, CMS integrations, etc.), and accomplishing art direction.

We needed functionality at Harry's for the new homepage hero on https://www.shopflamingo.com/ (you can resize the browser window to see the image change). We forked gatsby-image to add this functionality, and would love to contribute it back to gatsby-image directly.

The fork would still need a little bit more work (it's currently only implemented for fluid images), but I'm more than happy to open a PR to start that process! Would love any especially love any direction on the proposed APIs.

@KyleAMathews
Copy link
Contributor

Love this! Thanks for researching this and providing a great overview of the problem.

I tend to like more explicit prop names rather than overloading existing ones but option 2 is a pretty linear extension so perhaps ok. But definitely lean towards option 1.

@polarathene
Copy link
Contributor

It would be nice to have gatsby-image functionality split out into something like React Hooks for composing the component, which'd also make additions like this probably easier to contribute and maintain as this component continues to grow in functionality/size.

Option 2 makes sense to me and seems fine as long as docs are clear about it. Option 1 could be fine too(can always raise an error about the issue of new prop and existing source prop both being populated), though the suggested prop name doesn't seem like the right choice for the indicated feature.(since it creates a distinction between the two related props, it'd potentially cause some confusion).

@brimtown
Copy link
Contributor Author

brimtown commented Apr 8, 2019

Thanks @KyleAMathews !

@polarathene Agreed about splitting out the functionality, there's a ton of branching in the file currently that makes it difficult to add functionality cleanly. Once I put a PR up, maybe we can look at the diff together and see what opportunities there would be to refactor?

What would you suggest for naming for Option 2? It seems to me that the new props would want to have fluid- and fixed- as prefixes to show their connection to the existing props, since that will determine how they ultimately get styled. Maybe fluidImages and fixedImages?

I agree that it might cause some confusion, and I'm wondering about what additional edge cases adding new props introduces (what should we do if someone passes both fixed and fixedImages?). Option 1 seems to avoid these issues which is nice.

I'm happy to use this issue to keep iterating on the API, and I'll start on opening the PR.

@polarathene
Copy link
Contributor

@brimtown I don't have experience with this technique, if it's just using multiple image source elements and with a media attribute for each one, then how is it different to the current approach of providing an array of images to fluid/fixed props?

Wouldn't it be possible to pass the array of images the same way, and if a media array prop is also given, then add the images as multiple sources each with their matching media attribute? (presently there is only a single additional source for webp) A media prop would be a trigger for this feature, without requiring anything else specific to fluid/fixed?

In the past month or so, there was a commit regarding a Safari issue, which refactored the picture element, which dropped/moved some data from a source element into the img element I think, so you might want to look into that to avoid a regression if this feature would avoid the current approach(non webp data assigned to img element, webp to a source element).


Once I put a PR up, maybe we can look at the diff together and see what opportunities there would be to refactor?

I won't have the time to look at refactoring much unfortunately. I've got to finish up my portfolio and get some paid work heading my way. If Gatsby can support a min version of React for using the Hooks api(16.8+ I think), I think that'd be a good way to go, but I'm not sure the core team would agree on that requirement? Maybe for v3.

What would you suggest for naming for Option 2?

I prefer something like an extra single prop like mentioned earlier that doesn't need to be prefixed by fluid/fixed (I don't even know if we really need that vs a boolean/type prop for the current component, but that'd break the current API). If you insist on a fluid/fixed prefix, perhaps fluidVariants makes sense? Rather than just different sizes/densities, actual variations of the content are being provided.

what should we do if someone passes both fixed and fixedImages?

Throw an error on build I guess to inform the user. Ideally no similar named new prop would be added causing that confusion. I might be mistaken with how it compares to srcSet(which is a list of images of different sizes or formats afaik), are these two compatible or do they replace one another?

Option 1 seems to avoid these issues which is nice.

It does, but without any additions to the API, is it clear from looking at it's usage that you can differentiate between instances with this feature in use or not?

My preference would be getting gatsby-image to provide a default(and perhaps some other configurations) after being refactored into a more composable component like hooks allow for. For example, if community wanted to handle the styling separately with say regular CSS or styled-components/linaria instead of inline css with current approach. Or different placeholders, etc.

Someone had forked gatsby-image late 2018 and worked on a refactor(not hooks based) with it split into smaller components. It's bit out of sync now though. It'd be a fair bit of work to refactor well and get merged in, so I wouldn't worry about it and stick with option 2 probably.

@polarathene
Copy link
Contributor

{
  aspectRatio: 0.9025270758122743,
  base64: "data:image/webp;base64,UklGRpIAAABXRUJQVlA4IIYAAAAwBACdASoUABYAPtFYokuoJSMhsBgMAQAaCWQApzyAATMYqzoy6WJHiIAA/u6WPLuQDe/zBWkk7EpQbTMyrGOzrMSlCt4JXigacYvXZAUEEkrKqVuD4yIy3FBygVx0hpXe48+iuQxgEyWU0M3WpHVbhRNMgtHY83tNHv/5Gsjx83gcpUgAAA==",
  sizes: "(max-width: 500px) 100vw, 500px",
  src: "/static/99eef364a067f7c0632d816832929955/018a8/printExample.jpg",
  srcSet: "/static/99eef364a067f7c0632d816832929955/4600d/printExample.jpg 125w,↵/static/99eef364a067f7c0632d816832929955/fdf47/printExample.jpg 250w,↵/static/99eef364a067f7c0632d816832929955/018a8/printExample.jpg 500w",
  srcSetWebp: "/static/99eef364a067f7c0632d816832929955/3de86/printExample.webp 125w,↵/static/99eef364a067f7c0632d816832929955/04576/printExample.webp 250w,↵/static/99eef364a067f7c0632d816832929955/67857/printExample.webp 500w",
  srcWebp: "/static/99eef364a067f7c0632d816832929955/67857/printExample.webp"
}

This is example data that goes into current fluid prop. You can see how the sizes seems similar to the media attribute you mentioned? Followed by srcSet for the different URLs paired with a size.

How is this new feature comparing to that besides different images in srcSet vs size/density that's currently provided? As the variants can differ quite a bit, how should a placeholder be handled?

If the data for what you're proposing varies quite a bit from current fluid/fixed data, then I agree with a different prop/component(from recomposing/sharing parts of gatsby-image). Is it data that is still processed via sharp to provide fixed/fluid data?

@brimtown
Copy link
Contributor Author

brimtown commented Apr 10, 2019

Thanks for the response @polarathene ! Let me see if I can respond to each of the points you raised. Please let me know if I missed something 😄

What's the difference between srcSet and <source media="">?

This is a great question, and it essentially boils to "srcSet is for resolution switching, and <source> is for art direction". The Responsive Images guide is really helpful in this regard to understanding the difference between these two concepts!

My first implementation of this actually tried to just re-use srcSet like you suggested, but trying to do that breaks in all sorts of surprising ways because browsers treat those very differently.

Doing this with <source> actually allows us to combine resolution switching and art direction, so that we can serve different image crops at different breakpoints, but also have those crops switch resolution depending on the screen size. It's the best of both worlds, and ensures that users are seeing the correct crop at the correct breakpoint, served in a resolution appropriate for their screen width.

Why can't we use a single media prop?

Basically for the above reasons as well. The browser is already switching resolutions based on the srcSet, but we want multiple srcSets for those distinct crops. This is why we need FluidObject[] and FixedObject[] as new prop types, regardless of if they're shimmed into the existing fluid and fixed, or introduced as their own new separate props. To put it another way, media does not operate on srcSets, it operates on <sources> themselves.

To my knowledge, the CMS's that provide GraphQL fragments for gatsby-image also don't let you control what goes into srcSet, so from a content modeling perspective, providing multiple full images also makes sense there.

What does this mean for WebP support?

This approach preserves serving WebP with non-WebP fallbacks. @tbrannam who contributed that change is also on our team here at Harry's, and we use that feature heavily 😄 . Feel free to inspect the hero on https://www.shopflamingo.com for an example of art direction (and resolution switching) working with WebP.

How do we deal with placeholders?

The beauty of making everything a picture tag (including the placeholders) is that the browser will automatically decide which image to show (both crop and resolution), and we don't have to worry about it!

General Refactoring Ideas for gatsby-image

Agreed that some of the potential refactors you brought up would make sense, and that they'd probably be out of scope for adding this functionality. I'd be happy to contribute that after getting the functionality merged first! Still working on opening this PR now.

@polarathene
Copy link
Contributor

polarathene commented Apr 11, 2019

@brimtown Thanks for getting back to me with such a great response! Really appreciated 😃

That was a clear explanation for why srcSet is different and complimentary. The media prop would still be a single prop though? It's just you need an array of fluid/fixed data(existing prop) to pair with that instead now?

What does this mean for WebP support?

Also good to know about @tbrannam being on the team regarding my concern about the Safari commit. I wasn't too concerned about WebP support as we had that afaik prior to his commit, the commit had resolved an issue with Safari by dropping one of the <source> elements and moving it's properties directly onto the <Img> component/element.

If a single picture element is used, what happens here? Does the source applied to the Img element default to the smallest or largest(first/last) media value with the rest as source elements(*2 for WebP support)?

How do we deal with placeholders?

I guess this makes sense, although it can add to the page weight when placeholders are using base64 data multiplied by the amount of media variants. More so with tracedSVG. When the data isn't inlined, that's less of an issue.

Perhaps that's a minor concern, as it's unlikely they'd be many images using this feature on a page? Be sure not to add WebP here in your PR however, or be careful about it so that it's not to impact usage of gatsby-image not using art direction. I have contributed support for using WebP in base64 instead of jpg/png, but not both to avoid inline bloat, this allowed increasing the placeholder resolution(or having a lower pageweight) with fallback to bgcolour(Firefox and Chrome support WebP which is a decent reach).

General Refactoring Ideas for gatsby-image

How do you envision (or currently use) this feature? Is it like the current GraphQL for getting fluid/fixed source data, but instead you're supplying an array of those results to a single gatsby-image + media attribute data?

Is there a GraphQL fragment in use to reduce the repetition for the different input images into the query?(assuming they're effectively the same query) Or does the user need to do more work to prepare the data to provide to gatsby-image?

Is your PR to introduce variants to the current fluid/fixed logic support, or improve these existing two to accommodate art direction? If the latter it'd make sense to keep the fluid/fixed props and detect an array or object as input for how to behave? (although I'm also concerned about that getting messy if you have to handle both cases when the prop is referenced(as image))

// Logic is adjusted to expect `image` as an array, so the current data is converted to 
// an array internally
const image = Array.isArray(fluid) ? fluid : [fluid]
// Alternatively take the first / last element as default `image`, and conditionally 
// handle for array data
const image = Array.isArray(fluid) ? fluid.shift() : fluid
const variants = Array.isArray(fluid) && fluid.length > 0 ? fluid : null

Maybe something like that could work.

@polarathene
Copy link
Contributor

polarathene commented Apr 15, 2019

@brimtown If you're interested in discussing the refactor at a later point, I've raised an issue for it here.

@wardpeet
Copy link
Contributor

I've wanted to fix the gatsby-image as well for the same reasons as above. We've been waiting to do this until gatsby v3. We will refactor this into using forwardRef, hooks and split it up in some smaller components. We could already refactor it with HOCs to make it cleaner but I think it's better to just wait for v3.

Basically, we could only publish gatsby-image to v3 and be done with it but changing the dependency of react to 16.8.0 means most users will need to upgrade too as 99% of all gastby sites use gatsby-image.

@wardpeet wardpeet added type: bug An issue or pull request relating to a bug in Gatsby type: feature or enhancement labels Apr 15, 2019
@brimtown
Copy link
Contributor Author

PR is open over at #13395, would love to start getting feedback!

@DSchau noticed in #13397 that Gold Medal Flour also forked gatsby-image to support art direction, with a slightly different API than what has been talked about here: https://github.com/thadroe/art-direction-images-for-gatsby.

It looks like that approach only supports art directing between two images, and introduces a single breakpoint prop which gets interpolated into the media attribute by gatsby-image. Interesting to see different tradeoffs!

Do people have any preferences between "supporting two images / interpolating a single breakpoint" vs. "supporting an arbitrary number of images / arbitrary media conditions"? I like the flexibility provided by the latter, and I don't think it adds too much additional complexity to the API.

@polarathene
Copy link
Contributor

@brimtown I'll review.

@gatsbot
Copy link

gatsbot bot commented May 14, 2019

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.

If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contributefor more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label May 14, 2019
@brimtown
Copy link
Contributor Author

Bumping to make this unstale, just pushed some latest changes over at #13395 .

@wardpeet
Copy link
Contributor

sorry to kepe you waiting i'll review tomorrow @brimtown

@gatsbot
Copy link

gatsbot bot commented May 27, 2019

Hey again!

It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.

Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community!

@GreatState
Copy link

bump. Will this make it into gatsby-image?

@lannonbr
Copy link
Contributor

lannonbr commented Sep 4, 2019

@GreatState it looks like it was merged in with #13395

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale? Issue that may be closed soon due to the original author not responding any more. type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants