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-source-contentful): Remove default image transform width #14159

Merged
merged 17 commits into from
Jun 10, 2019
Merged

feat(gatsby-source-contentful): Remove default image transform width #14159

merged 17 commits into from
Jun 10, 2019

Conversation

LuudJanssen
Copy link
Contributor

Description

This PR resolves the width of an image automatically using the aspect ratio when only the height is given in an image transform. Previously, a default width was used resulting in a cropped image.

This PR also creates unit tests to test this behaviour.

Related Issues

Fixes #14156
Related to #6752
Related to #6954
Related to this pull request about removing the default width when using gatsby-transformer-sharp
Related to this pull request about calculating the right dimensions / aspect ratio based on the passed arguments when using gatsby-plugin-sharp

@LuudJanssen
Copy link
Contributor Author

I have no idea why the end-to-end test is failing. It's failing on the Gatsby Preview (Updating) "before each" hook for "updates when content is deleted" because it reached a timeout of 9999ms. Could any of the core contributors help me resolve this?

@LuudJanssen LuudJanssen changed the title Topics/gatsby source contentful remove default image width [gatsby-source-contentful] Remove default image transform width May 19, 2019
@LekoArts LekoArts changed the title [gatsby-source-contentful] Remove default image transform width feat(gatsby-source-contentful): Remove default image transform width May 20, 2019
@LekoArts
Copy link
Contributor

I re-run the CI, seems like it was a hiccup. What do you think about adding some notes to the README, e.g. about the defaults for width/height?

@LekoArts LekoArts added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label May 20, 2019
@LuudJanssen
Copy link
Contributor Author

@LekoArts, sure! I'll try to add the changes to the documentation as soon as possible!

@LuudJanssen
Copy link
Contributor Author

@LekoArts, I merged the master branch into my branch and added some documentation about the height parameter. I didn't change the gatsby-plugin-sharp documentation, as gatsby-source-datocms and gatsby-source-sanity don't support setting only the height parameter.

Should we file issues to those repositories to fix this difference?

Also, I noticed gatsby-source-contentful, gatsby-plugin-sharp, gatsby-transformer-sharp, gatsby-source-santiy and gatsby-source-datocms share a lot of code regarding the definition of the additional node types (like resize, fixed, fluid; see this definition for example) and utilties for resolvers (like calculating width based on height and vice versa). Would it be wise to create some sort of utility library? Or offload most of this work to some other part of Gatsby I don't know about?

@LuudJanssen
Copy link
Contributor Author

Also, @LekoArts, I'm experiencing build issues again.

@pieh
Copy link
Contributor

pieh commented Jun 10, 2019

Also, @LekoArts, I'm experiencing build issues again.

Sorry for that, sometimes development runtime tests are flaky. After restarting them we are good again.

@LekoArts, I merged the master branch into my branch and added some documentation about the height parameter. I didn't change the gatsby-plugin-sharp documentation, as gatsby-source-datocms and gatsby-source-sanity don't support setting only the height parameter.

Should we file issues to those repositories to fix this difference?

I think it's worth at least notifying datocms and sanity we did this change and they might apply it too for uniformity

Also, I noticed gatsby-source-contentful, gatsby-plugin-sharp, gatsby-transformer-sharp, gatsby-source-santiy and gatsby-source-datocms share a lot of code regarding the definition of the additional node types (like resize, fixed, fluid; see this definition for example) and utilties for resolvers (like calculating width based on height and vice versa). Would it be wise to create some sort of utility library? Or offload most of this work to some other part of Gatsby I don't know about?

There might be already lib for that. This could be good idea (depending how much that utility would abstract and make it easier to read code), but it's not critical to do

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @LuudJanssen for PR and patience with review

@pieh pieh merged commit d24e22f into gatsbyjs:master Jun 10, 2019
@gatsbot
Copy link

gatsbot bot commented Jun 10, 2019

Holy buckets, @LuudJanssen — 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!

@sidharthachatterjee
Copy link
Contributor

Published in gatsby-source-contentful@2.0.69

@LuudJanssen LuudJanssen deleted the topics/gatsby-source-contentful-remove-default-image-width branch June 12, 2019 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gatsby-source-contentful] Image transforms still have default width
4 participants