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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby-image): Fix fluid not respecting maxWidth/maxHeight #17006

Merged
merged 27 commits into from
May 6, 2020
Merged

fix(gatsby-image): Fix fluid not respecting maxWidth/maxHeight #17006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
46b0a88
apply presentation values on gatsby-image-wrapper div
sbardian Aug 23, 2019
caf5d3f
add presentation values to gatsby fluid fragments
sbardian Sep 13, 2019
e8b9241
add maxHeight and maxWidth to node
sbardian Sep 17, 2019
d955d42
calculate and return maxHeight and maxWidth
sbardian Sep 17, 2019
37d2ccb
add image wrapper container and apply maxWidth and maxHeight
sbardian Sep 17, 2019
bf63c74
update test snapshots
sbardian Sep 17, 2019
7d90d50
update fragments with maxWidth and maxHeight
sbardian Sep 17, 2019
9cac425
garrantee aspect ratio is set
sbardian Nov 14, 2019
ec74ea0
use string types instead of int
sbardian Nov 14, 2019
1a6f6b7
update gatsby-plugin-sharp snapshots
sbardian Nov 14, 2019
f7d2dfb
add comment to maxHeight/maxWidth aspect ratios logic
sbardian Nov 29, 2019
a87a213
update snapshot after rebase
sbardian Nov 29, 2019
926733c
update snapshots
sbardian Jan 20, 2020
f681a28
update selector to account for extra wrapper container div
sbardian Jan 20, 2020
d3070f6
update gatsby-plugin-sharp snapshot
sbardian Jan 22, 2020
731a59c
remove unneeded gatsby-image-wrapper-container
sbardian Mar 1, 2020
efe650a
revert querySelector and update snapshots
sbardian Mar 1, 2020
553b546
test: update snapshots for gatsby-image
sbardian Mar 25, 2020
fde9b51
fix: revert unintended change
sbardian Mar 25, 2020
71e1054
Merge branch 'master' into topics/gatsby-image
wardpeet Mar 31, 2020
40ae22e
Merge branch 'master' into topics/gatsby-image
wardpeet Apr 17, 2020
180fb8b
update code to not break behaviour for maxWidth
wardpeet May 4, 2020
84e32e5
update tests and fix mistake
wardpeet May 4, 2020
0c7d984
use presentation fragment instead
wardpeet May 5, 2020
1f32355
remove some code
wardpeet May 5, 2020
a8ff738
update readme
wardpeet May 5, 2020
ec23783
fix graphql fragment
wardpeet May 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 4 additions & 24 deletions packages/gatsby-image/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ Their fragments are:
- `GatsbyImageSharpFluid_withWebp`
- `GatsbyImageSharpFluid_withWebp_noBase64`
- `GatsbyImageSharpFluid_withWebp_tracedSVG`
- `GatsbyImageSharpFluidLimitPresentationSize`

### gatsby-source-contentful

Expand Down Expand Up @@ -318,37 +319,16 @@ prop. e.g. `<Img fluid={fluid} />`
### Avoiding stretched images using the fluid type

As mentioned previously, images using the _fluid_ type are stretched to
match the container's width. In the case where the image's width is smaller than the available viewport, the image will stretch to match the container, potentially leading to unwanted problems and worsened image quality.
match the container's width and height. In the case where the image's width or height is smaller than the available viewport, the image will stretch to match the container, potentially leading to unwanted problems and worsened image quality.

To counter this edge case one could wrap the _Img_ component in order to set a better, for that case, `maxWidth`:

```jsx
const NonStretchedImage = props => {
let normalizedProps = props
if (props.fluid && props.fluid.presentationWidth) {
normalizedProps = {
...props,
style: {
...(props.style || {}),
maxWidth: props.fluid.presentationWidth,
margin: "0 auto", // Used to center the image
},
}
}

return <Img {...normalizedProps} />
}
```

**Note:** The `GatsbyImageSharpFluid` fragment does not include `presentationWidth`.
You will need to add it in your graphql query as is shown in the following snippet:
To counter this edge case one could use the `GatsbyImageSharpFluidLimitPresentationSize` fragment to ask for additional presentation size properties.

```graphql
{
childImageSharp {
fluid(maxWidth: 500, quality: 100) {
...GatsbyImageSharpFluid
presentationWidth
...GatsbyImageSharpFluidLimitPresentationSize
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions packages/gatsby-image/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ const getImageSrcKey = ({ fluid, fixed }) => {

/**
* Returns the current src - Preferably with art-direction support.
* @param currentData {{media?: string}[]} The fluid or fixed image array.
* @return {{src: string, media?: string}}
* @param currentData {{media?: string}[], maxWidth?: Number, maxHeight?: Number} The fluid or fixed image array.
* @return {{src: string, media?: string, maxWidth?: Number, maxHeight?: Number}}
*/
const getCurrentSrcData = currentData => {
if (isBrowser && hasArtDirectionSupport(currentData)) {
Expand Down Expand Up @@ -480,6 +480,8 @@ class Image extends React.Component {
style={{
position: `relative`,
overflow: `hidden`,
maxWidth: image.maxWidth ? `${image.maxWidth}px` : null,
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
maxHeight: image.maxHeight ? `${image.maxHeight}px` : null,
...style,
}}
ref={this.handleRef}
Expand Down Expand Up @@ -717,6 +719,8 @@ const fluidObject = PropTypes.shape({
srcWebp: PropTypes.string,
srcSetWebp: PropTypes.string,
media: PropTypes.string,
maxWidth: PropTypes.number,
maxHeight: PropTypes.number,
})

// If you modify these propTypes, please don't forget to update following files as well:
Expand Down
4 changes: 2 additions & 2 deletions packages/gatsby-plugin-sharp/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,8 +621,8 @@ async function fluid({ file, args = {}, reporter, cache }) {
srcSet,
srcSetType,
sizes: options.sizes,
originalImg: originalImg,
originalName: originalName,
originalImg,
originalName,
density,
presentationWidth,
presentationHeight,
Expand Down
4 changes: 2 additions & 2 deletions packages/gatsby-transformer-sharp/src/customize-schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,8 @@ const fluidNodeType = ({
sizes: { type: new GraphQLNonNull(GraphQLString) },
originalImg: { type: GraphQLString },
originalName: { type: GraphQLString },
presentationWidth: { type: GraphQLInt },
presentationHeight: { type: GraphQLInt },
presentationWidth: { type: new GraphQLNonNull(GraphQLInt) },
presentationHeight: { type: new GraphQLNonNull(GraphQLInt) },
},
}),
args: {
Expand Down
11 changes: 11 additions & 0 deletions packages/gatsby-transformer-sharp/src/fragments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,17 @@ export const GatsbyImageSharpFluid = graphql`
}
`

/**
* Presentation sizes to make sure a fluid container does not overflow
* @type {Fragment}
*/
export const GatsbyImageSharpFluidLimitPresentationSize = graphql`
fragment GatsbyImageSharpFluidLimitPresentationSize on ImageSharpFluid {
maxHeight: presentationHeight
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wardpeet if you are setting the maxHeight/maxWidth to the presentationHeight/presentationWidth I'm not sure this will end up being correct, see this comment of mine on the issue of using presentationHeight/Width as maxHeight/Width. comment. Basically the presentationHeight/Width max out at the images dimensions, so if your image is 800x800 and you set maxWidth to 1500, it will still only be maxWidth 800.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this comment also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was finally able to get the fragments to update and working using:

       childImageSharp {
          fluid(maxWidth: 500) {
            ...GatsbyImageSharpFluid
            ...GatsbyImageSharpFluidLimitPresentationSize
          }
        }

and it does appear to be using presentationHeight/Width. So any maxHeight/width will max out at the image dimensions. This is why I had the logic in gatsby-plugin-sharp

  let maxWidth
  let maxHeight
  if (options.maxHeight || options.maxWidth) {
    maxWidth = options.maxWidth
      ? `${options.maxWidth}px`
      : `${options.aspectRatio * options.maxHeight}px`
    maxHeight = options.maxHeight
      ? `${options.maxHeight}px`
      : `${options.aspectRatio * options.maxWidth}px`
  }

and specifically returned maxHeight/maxWidth it as part of the fluid return object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting it to presentationWidth/height feels more correct as your the container shouldn't be bigger than image. If you need that, you'll need a bigger image, else you'll end up with quality drop.

If you want to center the image or override the behavior you can still add a wrapper or set the maxWidth/maxHeight yourself on gatsby-image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this makes more sense I'm good with it.

maxWidth: presentationWidth
}
`

/**
* Traced SVG fluid images
* @type {Fragment}
Expand Down