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-transformer-sharp): add inside and outside fit options #13393

Closed
wants to merge 2 commits into from

Conversation

VGoose
Copy link
Contributor

@VGoose VGoose commented Apr 16, 2019

Description

Add inside and outside options

Related Issues

Addresses #12972

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Could you provide a sample use case for this and/or project configured to use these options?

@DSchau DSchau changed the title add inside and outside fit options feat(gatsby-transformer-sharp): add inside and outside fit options Apr 16, 2019
@VGoose
Copy link
Contributor Author

VGoose commented Apr 16, 2019

Here's a use case for inside #13078 (comment).

outside: resizing images to be at least a minimum dimension

@VGoose
Copy link
Contributor Author

VGoose commented Apr 16, 2019

I'm thinking of moving fit and background options from the fluid method to shared options that can be used by resize and fixed. I remember thinking about this for PR #13078 but then deciding against it. I can't remember why..

Would love some feedback on this point.

@pieh
Copy link
Contributor

pieh commented Apr 16, 2019

I think we will need to make more changes than just adding those options in transformer - it seems like inside and outside forces different aspect ratio, than one that would be calculated by gatsby itself

@VGoose
Copy link
Contributor Author

VGoose commented Apr 16, 2019

Could you clarify what you mean by gatsby itself?

Sharp is making the correct image sizes with inside and outside as far as I can tell from inspecting the static folder. As for displaying the image correctly with gatsby-image, I was thinking we could handle that with documentation.

@pieh
Copy link
Contributor

pieh commented Apr 16, 2019

Could you clarify what you mean by gatsby itself?

Sharp is making the correct image sizes with inside and outside as far as I can tell from inspecting the static folder. As for displaying the image correctly with gatsby-image, I was thinking we could handle that with documentation.

I think I might misunderstood what fit options do in sharp (it would be super helpful if sharp docs had images to showcase what the option does :D), for now please discard my comment (at least until I ran some tests)

@pieh
Copy link
Contributor

pieh commented Apr 16, 2019

Ok, I was actually correct (that's new :D):

Input image here was 4823 x 7234 (~portraity aspect ratio). I've run sharp using all fit options and using square width/height for resize (570x570)

Screenshot 2019-04-16 at 23 51 20

the cover, contain and fill options all produce same output width/height, but inside and outside modifies it to maintain aspect ratio of input image.

We calculate width/height (for fixed/resize) or aspectRatio (for fluid) in gatsby-plugin-sharp so gatsby-image can size DOM elements properly. So sharp will produce images like you would expect, but gatsby-image still would use square aspect ratio (because of information it got from gatsby-plugin-sharp)

@VGoose
Copy link
Contributor Author

VGoose commented Apr 16, 2019

I think inside can be applied across the 3 methods without any code changes except for documentation.

Both the width/height (for fixed/resize) or aspectRatio (for fluid) behave naturally, creating a "frame" around the image. While the image is a different aspect ratio, the "frame" is controlled by the respective input.

We can make outside only be available for resize, the width/height (for fixed/resize) or aspectRatio (for fluid) would not be meaningful if this was applied to fluid or fixed.

@pieh
Copy link
Contributor

pieh commented Apr 16, 2019

Hmm, but won't end result (if we don't change width/height and aspectRatio calcs) look exactly the same for cover, inside and outside fit options?

@VGoose
Copy link
Contributor Author

VGoose commented Apr 16, 2019

Good point! Right now gatsby-image's Img has default objectFit: 'cover'. If we pass imgStyle={{ objectFit: 'contain' } to its props we get the desired result.

I was thinking of adding documentation to point this out because gatsby-image doesn't get the fit option information from gatsby-plugin-sharp currently.

@pieh pieh added the status: awaiting author response Additional information has been requested from the author label Jun 10, 2019
@pieh
Copy link
Contributor

pieh commented Jun 10, 2019

Hey @VGoose. Did you have any time to work more on this or do you think you will have time to finish this Pull Request? We would love to have this feature in. Let us know if you need any help with finishing this up 💜 💪 !

@VGoose
Copy link
Contributor Author

VGoose commented Jun 10, 2019

Hi @pieh, I recently got a new job and currently don't have the bandwidth to do this :(. It's something I really want to add but unfortunately I really can't commit to it right now. 💔

@pieh
Copy link
Contributor

pieh commented Jun 10, 2019

Hi @pieh, I recently got a new job and currently don't have the bandwidth to do this :(. It's something I really want to add but unfortunately I really can't commit to it right now. 💔

That's totally understandable - we appreciate effort you put in this and Your previous PRs 💜 💪. For now let's close the PR if you don't have bandwidth to work on this - we can always reopen it later on.

@pieh pieh closed this Jun 10, 2019
@chris-hammond
Copy link
Contributor

Hi @pieh, Hi @VGoose,

I've been waiting for the PR to merge since April! What needs to be done to finish it? From your conversation, it sounds like it's just documentation changes that remain? Or were you planning on trying to pass the fit option from gatsby-plugin-sharp to gatsby-image?

I'm happy to do the work to get this ready to merge, but I'd appreciate guidance from the two of you on the path we want to take!

@VGoose
Copy link
Contributor Author

VGoose commented Jun 11, 2019

Hi @chris-hammond, from my last conversation with @pieh, we wanted to ship these options for all three methods. There are a few small issues but the main issue is that is blocking this (for me) is fluid's arguments (maxWidth and maxHeight) need to be ironed out first.

With fit set to outside, the final image actually can be higher thanmaxWidth and maxHeight. There needs to be documentation for what exactly the intent of these arguments are or a maybe even a renaming (internally a fluid call is using sharp's resize method - gatsby does some transformations of mW and mH before passing it as width and height to resize).

wardpeet added a commit that referenced this pull request Mar 10, 2020
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants