-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[pls review] Fix contentful image handling #4207
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
[pls review] Fix contentful image handling #4207
Conversation
axe312ger
commented
Feb 23, 2018
- Export the schemes for [Ready 4 Review] SQIP - Vectorized primitive image previews #4205
- Allow background image setting for proper output when cropping
- Return correct aspect ratio for responsive image nodes
|
Deploy preview for gatsbygram ready! Built with commit 03aa963 |
|
@Khaledgarbaya maybe u can have a look :) |
Khaledgarbaya
left a comment
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.
LGTM
KyleAMathews
left a comment
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.
Nice work!
| const resp = await resolveResponsiveSizes(image, { | ||
| maxWidth: 450, | ||
| maxHeight: 399, | ||
| maxHeight: 400, |
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.
Could you change this back to 399? It was set this way to test that rounding works I believe.
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.
Ohh, felt so weird to me to have such an non-practical height and a resulting aspect ratio of 1.1278195488721805
but yes, I can do that :)
| width: 450, | ||
| height: 399, | ||
| quality: 50, | ||
| background: `rgb:000000`, |
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.
What's this for?
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.
The background is set when doing cropping operations where the source image does not completely cover the result.
See:
https://images.contentful.com/wln3fygpfpvt/7orLdboQQowIUs22KAW4U/6c709893ae83938bfef443a7e5f8c658/matt-palmer-254999.jpg?w=1180&h=480&fit=pad
https://images.contentful.com/wln3fygpfpvt/7orLdboQQowIUs22KAW4U/6c709893ae83938bfef443a7e5f8c658/matt-palmer-254999.jpg?w=1180&h=480&fit=pad&bg=rgb:000000
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.
Ah cool 👍
87a10d9 to
03aa963
Compare
|
@KyleAMathews this is ready for re-review :) |
| exports[`contentful extend node type resolveResponsiveSizes generates responsive sizes data for images using all options 1`] = ` | ||
| Object { | ||
| "aspectRatio": 0.75, | ||
| "aspectRatio": 1.1278195488721805, |
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 like something broke this?
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 the outcome of the bugfix for this result w=450&h=399 b0c658c
It returned the original aspect ratio while the resulting one should be returned
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.
ahhh :-) cool!
|
Thanks! |

