-
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(*): add options fit and background to image sharp #13078
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.
This is starting to look good! I added a few nits about the README.
Still need to test it myself but code looks 👌
Failing ci/circleci: starters_validate
I'm not sure how to fix this, all tests pass locally for me. |
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.
This looks good to me! Let me this one before merging
Holy buckets, @VGoose — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
This unfortunately doesn't quite cover the use case I had in mind. In "contain" mode, with "maxWidth" and "maxHeight" both set, I was expecting the output images to still be the source image's aspect ratio, not forced to the ratio maxWidth:maxHeight and with a background colour applied. Example:
I expected the output for these particular images to be 200x100 pixels (along with smaller and bigger versions for different size viewports and pixel densities, eg 100x50, 150x75, 300x150, 400x200) and 100x200 pixels (along with smaller and bigger versions): the original aspect ratios, but with the "canonical" size fitting within the 200x200 I specified as maximum. But with this patch I get two 200x200 images with black bars baked in (along with smaller and bigger versions, all with black bars). It looks like the background could be set to transparent so you don't get actual black bars, but you're still changing the aspect of the output image from the images' source aspects, which to me is undesirable. Imagine for example another scenario where instead of showing these images in a grid they're floating in a block of text, and you want the text to flow around them. |
@tremby thanks for the clear example! When I was making this PR I left **Note for others who might read this: with |
That's a really useful link. Yes, what I'm looking for and would have expected for "contain" is what they call "inside". I expect there are valid use cases for both, and I see no reason to break from their terminology. :) While I can't think of a use case for "outside" off the top of my head, I do imagine one exists! |
Yep! I'll submit a PR once it gets completed. |
Added `INSIDE` and `OUTSIDE` options for for gatsby-transformer-sharp. Here's a use case for inside [#13078 (comment)](#13078 (comment)). This includes changing gatsby-image to use `contain` instead of `cover` for object-fit, which does not affect the existing fit options. Updated documentation for gatsby-transformer-sharp to clarify the fit options and show that they are available for all three functions. NOTE: This is an extension of PR #13393 by @VGoose , which was closed because he didn't have time to complete the work. Co-authored-by: AnhVoMBP15 <[email protected]> Co-authored-by: soundguy66 <[email protected]> Co-authored-by: gatsbybot <[email protected]> Co-authored-by: Ward Peeters <[email protected]>
Description
gatsby-plugin-sharp
fit
andbackground
options to control cropping when using fluidgatsby-transformer-sharp
ImageFitType
type and addfit
andbackground
tofluidNodeType
args.Related Issues
Addresses #12972.