gatsby-plugin-sharp fluid maxWidth maxHeight misleading

[VGoose]

Description

• maxWidth and maxHeight will control aspect ratio/cropping if both are provided but this isn't documented
• sizes is an option for fluid but not documented
• not clear that by default maxWidth or maxHeight is used to determine the source break points - leads to poorly optimized breakpoints if maxWidth or maxHeight is close to or over original size.
• when cropped, placeholders remain at original aspect ratio #12008

Steps to reproduce

  gatsby new fluid-test https://github.com/gatsbyjs/gatsby-starter-default

in src/components/image.js, call fluid like so:

 fluid(maxWidth: 500, maxHeight: 400)

Expected result

Expect maxWidth and maxHeight to control the max image size made by fluid.

Actual result

• Image gets cropped.
• Placeholder is still 1:1 aspect ratio

Environment

• Firefox 66
• Chrome 73

Proposal

• add cropping behavior option
• add additional option for cropping, instead of using maxWidth and maxHeight
• update docs

11851

[missmatsuko]

I think that using maxWidth and maxHeight to set aspect ratio shouldn't be documented, but rather the behaviour changed and crop or cover option added to address both the situations in https://github.com/gatsbyjs/gatsby/issues/11851#issuecomment-478279744

[tremby]

I think you might be misunderstanding the sizes attribute. It's expected and desirable that some of the assets Gatsby produces are larger than the maxWidth and maxHeight parameters.

With those width and height parameters you're supposed to be expressing the largest width and height you're going to display the image at, in CSS pixels.

But your browser will choose a source image to make use of as many physical pixels as it can. If your display is high resolution (like most current mobile phones, and many current laptops), this will often be an image with a larger resolution than the presentation size you dictated to Gatsby.

[tremby]

My suggestion is as @missmatsuko said above, to add an option for cropping behaviour: cover or contain (and possibly further options for cover, to control how the cropping happens), and for now to set the default to cover, since it sounds like that's the current behaviour.

At the next major version bump, that default can change to contain, which I'd argue is the expected behaviour.

[VGoose]

@tremby I didn't know about physical pixels vs CSS pixels before. This makes a lot more sense now.

I agree that the real issue now is the misleading nature of maxHeight and maxWidth and what they do with cropping.

I would add that we make it obvious srcset is created using maxWidth/maxHeight and filtered by original size (only go up to) by default. This would help for cases where people set a maxWidth/maxHeight close to the original and so there ends up only being a few breakpoints before hitting the max.

Also add that the sizes option to fluid should be documented, too. Currently it's an accepted argument in the source but not documented. But maybe this should be separate to this issue.

[tremby]

It's not just maxWidth and maxHeight, it's also srcSetBreakpoints; see https://www.gatsbyjs.org/packages/gatsby-plugin-sharp#fluid for details.

[VGoose]

Sorry I meant for the default, when srcSetBreakPoints is not provided.