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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby-plugin-sharp): return correct height #25150

Merged
merged 7 commits into from
Jul 19, 2020
Merged

fix(gatsby-plugin-sharp): return correct height #25150

merged 7 commits into from
Jul 19, 2020

Conversation

imakida
Copy link
Contributor

@imakida imakida commented Jun 20, 2020

When both a maxWidth and maxHeight option is provided, the height of the
image should be determined by the height and width of the image if the
image is smaller than the provided maxWidth and maxHeight options. i.e.
if the maxWidth and maxHeight option is 3840 and 2160, but the image is
720 x 1080, the maxHeight should be 1080.

fixes #25149

Description

Documentation

Related Issues

@imakida imakida requested a review from a team as a code owner June 20, 2020 08:10
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 20, 2020
@imakida
fix(gatsby-plugin-sharp): return correct height
2e78671 
When both a maxWidth and maxHeight option is provided, the height of the
image should be determined by the height and width of the image if the
image is smaller than the provided maxWidth and maxHeight options. i.e.
if the maxWidth and maxHeight option is 3840 and 2160, but the image is
720 x 1080, the maxHeight should be 1080.

fixes gatsbyjs#25149
polarathene
polarathene suggested changes Jun 20, 2020
View reviewed changes
Copy link
Contributor

@polarathene polarathene left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, just need to update the fixed version too. Thanks for finding the bug and sending through a PR! 😀

packages/gatsby-plugin-sharp/src/index.js Outdated Show resolved Hide resolved
polarathene
polarathene previously approved these changes Jun 21, 2020
View reviewed changes
@freiksenet freiksenet added topic: sharp and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 22, 2020
@polarathene polarathene mentioned this pull request Jul 3, 2020
Closed
7 tasks
wardpeet
wardpeet approved these changes Jul 3, 2020
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

😱 Oh my, awesome catch! Thank you 🙏 you rock! 💯

wardpeet
wardpeet approved these changes Jul 3, 2020
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

😱 Oh my, awesome catch! Thank you 🙏 you rock! 💯

wardpeet
wardpeet previously approved these changes Jul 3, 2020
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

😱 Oh my, awesome catch! Thank you 🙏 you rock! 💯

@polarathene
Copy link
Contributor

@imakida tests are failing, you'll need to update the snapshots.

eg This sharp test failing due to height values differing from current snapshot:

 gatsby-plugin-sharp › fluid › calculate height based on width when maxWidth & maxHeight are present
    expect(received).toMatchSnapshot()
    Snapshot name: `gatsby-plugin-sharp fluid calculate height based on width when maxWidth & maxHeight are present 5`

packages/gatsby-plugin-sharp/src/__tests__/index.js:289:60

@imakida
Merge branch 'master' into fix-25149
f2de2be
@imakida
Merge branch 'fix-25149' of github.com:imakida/gatsby into fix-25149
898928b
@imakida
fix(gatsby-plugin-sharp): return correct height
04e21bf 
Return correct height when both `maxHeight` and `maxWidth` are provided
for fluid images. We need to distinguish based on the `option`,
specifically, `INSIDE` and `COVER` (default if none provided).

fixes gatsbyjs#25149
@imakida imakida dismissed stale reviews from wardpeet and polarathene via 04e21bf July 3, 2020 21:17
@imakida
test(gatsby-plugin-sharp) update Jest snapshot
89ab390 
Update snapshot to expect the correct height for when the fit is
"inside".

fix gatsbyjs#25149
@imakida
Copy link
Contributor Author

imakida commented Jul 4, 2020

@polarathene I made changes to correct the issue with the default fit: COVER option for fluid and updated the Jest snapshot for the fit: INSIDE option. Tests are passing now.

polarathene
polarathene approved these changes Jul 4, 2020
View reviewed changes
Copy link
Contributor

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Great, thanks for sorting out those tests!

@wardpeet will merge it after the weekend I assume 😀

@polarathene
Copy link
Contributor

@wardpeet Please merge? :)

wardpeet
wardpeet approved these changes Jul 19, 2020
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Thank you! Sorry to keep you waiting.

@wardpeet wardpeet merged commit f1ace29 into gatsbyjs:master Jul 19, 2020
@gatsbot
Copy link

gatsbot bot commented Jul 19, 2020

Holy buckets, @imakida — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polarathene polarathene polarathene approved these changes

@wardpeet wardpeet wardpeet approved these changes

Assignees

@wardpeet wardpeet

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gatsby-plugin-sharp fluid image not returning proper sizes with given both maxHeight and maxWidth
4 participants
@imakida @polarathene @wardpeet @freiksenet