remark-images blows up images to 100% width and ignores ppi #1987
Conversation
|
Totally forgot that there is an auto deploy for Gatsby. I probably didn't have to record a video :) |
|
Deploy preview ready! Built with commit 8c67eb9 |
|
Deploy preview ready! Built with commit 27dee8f |
There was a problem hiding this comment.
Looking good! Thanks for fixing things!
using-remark looks nice https://deploy-preview-1987--using-remark.netlify.com/responsive-images-and-iframes/
as does gatsbyjs.org https://deploy-preview-1987--gatsbyjs.netlify.com/blog/gatsby-first-beta-release/
| @@ -363,6 +363,13 @@ async function responsiveSizes({ file, args = {} }) { | |||
| options.sizes = `(max-width: ${options.maxWidth}px) 100vw, ${options.maxWidth}px` | |||
| } | |||
|
|
|||
| // Account for images with a high pixel density. We assume that these types | |||
| // of images are intended to be used as high resolution ("retina") images. | |||
There was a problem hiding this comment.
Perhaps change the comment to:
"We assume that these types of images are intended to be displayed at their native resolution."
The sentence as it reads now is confusing because we always aim to provide users with retina-class devices a high resolution image. The fix is to avoid expanding the images themselves beyond their original width/height.
There was a problem hiding this comment.
Good point. Updated the comment!
| @@ -363,6 +363,13 @@ async function responsiveSizes({ file, args = {} }) { | |||
| options.sizes = `(max-width: ${options.maxWidth}px) 100vw, ${options.maxWidth}px` | |||
| } | |||
|
|
|||
| // Account for images with a high pixel density. We assume that these types | |||
| // of images are intended to be used as high resolution ("retina") images. | |||
| const { width, height, density } = await sharp(file.absolutePath).metadata() | |||
There was a problem hiding this comment.
It'd be much more efficient to get the metadata from the previous sharp instance for the file. There's costs to making this call as it has to read the image off disk, do analysis, etc.
There was a problem hiding this comment.
Which previous sharp instance do you mean? AFAIK this is the first one. I could only try to pass this instance down to gatsby-plugin-sharp.
|
Videos are always super nice though :-) |
Then I'll keep them coming :). Here is a new one which shows how the browser is selecting larger image variants while taking pixel ratio into account (I had no idea it would do this automatically, but I suppose this is why the width definition in https://www.youtube.com/watch?v=pRP6woILpsE&feature=youtu.be As far as I am concerned, this PR is now doing what we discussed. I (currently) have no more open action points. |
|
Tested my changes also in my company's docs. Looking much better now! |
|
Found something which looks suspicious in our docs. Might be that I have overlooked a certain sizing case. Will update once I have analyzed this. Pls don't merge yet. |
|
Okay, the suspicious behavior was actually caused by an image that got replaced (we had a few broken PNGs in our docs). I was confused by that. These changes work as intended 👍. |
|
Nice! Thanks for taking this from issue to very nice PR! |
|
Deploy preview failed. Built with commit 27dee8f https://app.netlify.com/sites/using-glamor/deploys/59a9d94b0752d016b2164dc3 |
|
Sorry for that. I thought that |
|
Having some trouble with this patch, as described in #2292 |
As discussed in #1982, changes the following:
I still have to validate this in more detail, mostly the
srcsetattribute and how it is working. The resizing behavior seems to work fine though. Open for early feedback should you find a minute @fk / @KyleAMathews.Quick video of the resizing / fallback behavior: https://www.youtube.com/watch?v=hDgrbNcCi1I&feature=youtu.be
Fixes #1982