-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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-image): Add support for backgroundColor in sharp #29141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks solid. Left a few comments but should be good when those are addressed.
@@ -118,11 +119,11 @@ export async function generateImageData({ | |||
reporter.warn( | |||
`Specifying fullWidth images will ignore the width and height arguments, you may want a constrained image instead. Otherwise, use the breakpoints argument.` | |||
) | |||
args.width = metadata.width | |||
args.width = metadata?.width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a scenario where metadata will be undefined entirely? It looks like in the worst case it's an empty object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If sharp throws an error getting metadata it would be undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change it so it's an empty object though
...args.webpOptions, | ||
width, | ||
height: Math.round(width / imageSizes.aspectRatio), | ||
toFormat: `webp`, | ||
background: backgroundColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we passing this if we already have sharedOptions? That seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Left that in after I refactored the others into sharedOptions
toFormatBase64: args.blurredOptions?.toFormat, | ||
width: placeholderWidth, | ||
height: Math.round(placeholderWidth / imageSizes.aspectRatio), | ||
background: backgroundColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above. sharedOptions is already passed.
@@ -504,9 +504,8 @@ const imageNodeType = ({ | |||
type: TransformOptionsType, | |||
description: `Options to pass to sharp to control cropping and other image manipulations.`, | |||
}, | |||
background: { | |||
backgroundColor: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we have any other doc where this needs to be changed? I don't think so, but wanted to mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think we mention it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Unifies API to support backgroundColor on both the component and the resolver. This is a breaking API change, as poreviously the resolver took
background
. However that was not actually implemented, so was ignored.New behaviour is to pass the prop to sharp, so when resizing with a crop fit of e.g.
contain
, sharp uses that as the background color for letterboxing. The value is also set as the background color of the container, meaning that if the image has transparency, if there is no placeholder, or the placeholder is tracedSVG then the background is visible.[ch22667]