-
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
fix(gatsby-plugin-image): Fix handling of sizes prop in SSR #28835
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.
Deja-vu ;)
The spread will pass on an unknown set of properties. I'd prefer to see them set explicitly. But not blocking you on this. Feel free to ignore the suggestion.
@@ -49,7 +49,6 @@ export const GatsbyImage: FunctionComponent<GatsbyImageProps> = function GatsbyI | |||
layout, |
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.
layout, | |
layout, | |
fallback, |
if (images.fallback) { | ||
cleanedImages.fallback = { | ||
src: images.fallback.src, | ||
...images.fallback, |
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 (images.fallback) { | |
cleanedImages.fallback = { | |
src: images.fallback.src, | |
...images.fallback, | |
if (fallback) { | |
cleanedImages.fallback = { | |
...fallback, |
… into fix/image-ssr-sizes-prop
6883d93
to
8f4482f
Compare
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.
In general this looks good. I'd like to include an e2e test with sizes as a prop since that case isn't covered and this feels like a good opportunity.
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.
Thanks for the test additions. Is it still the case that with some sizes data will override the automatic sizes? That's the main use case I'm worried we waren't testing.
@laurieontech Ah, gotcha. Added a test for that |
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.
Looks great!
* fix(gatsby-plugin-image): Fix handling of sizes prop in SSR * Update tests * Update tests * Fix types * Update e2e tests * Update e2e-tests/gatsby-static-image/src/pages/constrained.js Co-authored-by: LB <[email protected]> * Add test for size override Co-authored-by: gatsbybot <[email protected]> Co-authored-by: LB <[email protected]> (cherry picked from commit a135c50)
…28867) * fix(gatsby-plugin-image): Fix handling of sizes prop in SSR * Update tests * Update tests * Fix types * Update e2e tests * Update e2e-tests/gatsby-static-image/src/pages/constrained.js Co-authored-by: LB <[email protected]> * Add test for size override Co-authored-by: gatsbybot <[email protected]> Co-authored-by: LB <[email protected]> (cherry picked from commit a135c50) Co-authored-by: Matt Kane <[email protected]>
The sizes prop was being incorrectly handled in SSR. It should be using the property of the fallback image, not a top-level sizes prop.