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

feat(gatsby-plugin-utils): support aspect ratio for Image Service #35087

Merged
merged 7 commits into from
Mar 23, 2022

Conversation

veryspry
Copy link
Contributor

@veryspry veryspry commented Mar 9, 2022

Description

We were not passing along the aspectRatio arg passed into the GatsbyImage field / resolver to the underlying query string arg builder and thus we weren't actually doing anything with aspectRatio. This fixes that and adds a couple tests!

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 9, 2022
@veryspry veryspry changed the title feat: support aspect ratio in image cdn feat: support aspect ratio for Image Service Mar 9, 2022
@wardpeet
Copy link
Contributor

wardpeet commented Mar 9, 2022

We should use aspectRatio when calculating crop & size instead of passing it through sharp.

export function setDefaultDimensions(
args: IGatsbyImageHelperArgs
): IGatsbyImageHelperArgs {
let {
layout = `constrained`,
width,
height,
sourceMetadata,
breakpoints,
aspectRatio,
formats = [`auto`, `webp`],
} = args
formats = formats.map(format => format.toLowerCase() as ImageFormat)
layout = camelCase(layout) as Layout
if (width && height) {
return { ...args, formats, layout, aspectRatio: width / height }
}
if (sourceMetadata.width && sourceMetadata.height && !aspectRatio) {
aspectRatio = sourceMetadata.width / sourceMetadata.height
}
if (layout === `fullWidth`) {
width = width || sourceMetadata.width || breakpoints[breakpoints.length - 1]
height = height || Math.round(width / (aspectRatio || DEFAULT_ASPECT_RATIO))
} else {
if (!width) {
if (height && aspectRatio) {
width = height * aspectRatio
} else if (sourceMetadata.width) {
width = sourceMetadata.width
} else if (height) {
width = Math.round(height / DEFAULT_ASPECT_RATIO)
} else {
width = DEFAULT_FIXED_WIDTH
}
}
if (aspectRatio && !height) {
height = Math.round(width / aspectRatio)
} else if (!aspectRatio) {
aspectRatio = width / height
}
}
return { ...args, width, height, aspectRatio, layout, formats }
}

@@ -76,6 +80,66 @@ describe(`gatsbyImageData`, () => {
},
}

it(`should return proper image props for aspect ratio and automatically add "fit" prop`, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay for adding tests!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙌

@LekoArts LekoArts 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 Mar 10, 2022
@LekoArts LekoArts changed the title feat: support aspect ratio for Image Service feat(gatsby-plugin-utils): support aspect ratio for Image Service Mar 10, 2022
@veryspry
Copy link
Contributor Author

We should use aspectRatio when calculating crop & size instead of passing it through sharp.

export function setDefaultDimensions(
args: IGatsbyImageHelperArgs
): IGatsbyImageHelperArgs {
let {
layout = `constrained`,
width,
height,
sourceMetadata,
breakpoints,
aspectRatio,
formats = [`auto`, `webp`],
} = args
formats = formats.map(format => format.toLowerCase() as ImageFormat)
layout = camelCase(layout) as Layout
if (width && height) {
return { ...args, formats, layout, aspectRatio: width / height }
}
if (sourceMetadata.width && sourceMetadata.height && !aspectRatio) {
aspectRatio = sourceMetadata.width / sourceMetadata.height
}
if (layout === `fullWidth`) {
width = width || sourceMetadata.width || breakpoints[breakpoints.length - 1]
height = height || Math.round(width / (aspectRatio || DEFAULT_ASPECT_RATIO))
} else {
if (!width) {
if (height && aspectRatio) {
width = height * aspectRatio
} else if (sourceMetadata.width) {
width = sourceMetadata.width
} else if (height) {
width = Math.round(height / DEFAULT_ASPECT_RATIO)
} else {
width = DEFAULT_FIXED_WIDTH
}
}
if (aspectRatio && !height) {
height = Math.round(width / aspectRatio)
} else if (!aspectRatio) {
aspectRatio = width / height
}
}
return { ...args, width, height, aspectRatio, layout, formats }
}

Got it! I have a couple Q's about that:

  1. Is the best place to do that in the setDefaultDimensions function?
  2. Do I need to do anything further to make sure that aspectRatio doesn't get passed through sharp other than using it to calculate crop and size?

@wardpeet
Copy link
Contributor

TylerBarnes
TylerBarnes previously approved these changes Mar 14, 2022
@cameronbraid
Copy link
Contributor

both packages/gatsby-plugin-utils/src/polyfill-remote-file/http-routes.ts and packages/gatsby-plugin-utils/src/polyfill-remote-file/transform-images.ts need to be updated as to use the new fit and aspectRatio values

if (aspectRatio) {
// crop must be set for aspectRatio to work
if (!cropFocus) {
args.push(`fit=crop`)
Copy link
Contributor

@cameronbraid cameronbraid Mar 14, 2022

Choose a reason for hiding this comment

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

There is already a fit parameter in gatsbyImageResolver that should be used instead of the hard coded fit=crop.

Also - fit should be set even if there isn't an aspect ratio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually a bit of a special case. We use fit only to calculate width and height` so that locally processed images and those processed with Image CDN on Gatsby Cloud are identical in terms of size and crop.

Because of the way that aspectRatio is handled on Gatsby Cloud, it is required that fit be either "crop" or "fill". Here I make sure that the fit param is added if it does not exist already so that aspectRatio will actually be used on the Gatsby Cloud side

@@ -28,10 +28,12 @@ export function generateImageArgs({
format,
cropFocus,
quality,
aspectRatio,
Copy link
Contributor

@cameronbraid cameronbraid Mar 14, 2022

Choose a reason for hiding this comment

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

it would also be good to pass along the backgroundColor via the url so that when people use fit=contain the letterboxing can be done with the appropriate colour

@veryspry veryspry force-pushed the veryspry/image-cdn-aspect-ratio branch from 43b3167 to 964b373 Compare March 22, 2022 21:30
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Mar 23, 2022
@gatsbybot gatsbybot merged commit 3f2bc25 into master Mar 23, 2022
@gatsbybot gatsbybot deleted the veryspry/image-cdn-aspect-ratio branch March 23, 2022 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: media Related to gatsby-plugin-image, or general image/media processing topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants