-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
gatsby-plugin-sharp responsiveSizes maxWidth resizes to larger than the original #1509
Comments
Is it making larger images or is your css stretching it? I thought gatsby-plugin-sharp sharp doesn't resize upwards by default. |
I just took a (quick) look and can't reproduce what @chiedo is describing. I have a source image that is 400x300px, and using responsiveSizes either directly with |
Yes @KyleAMathews I'm pretty sure. It made an image that was 300 x 250 into an image that is 1920 x 1600. I'm getting that from src. |
This happened when using the following Graphql query
|
:/ … weird. Does the 1920x1600px image exist in |
@fk I assume so since the browser was rendering it and I was able to check what size it was. |
Does that not happen for you? |
(Sorry, AFK for a while) No, at least not with the one 400x300 JPG I tried. Is yours a JPG, too?
Also, at least Chrome should show you the rendered (and possibly blown up like Kyle supposed in his answer) as well as the original size of the image. Please take a look at the generated file name in your browser and then find it in public/static - is there an actual image that has the size you are seeing?
… Am 14.07.2017 um 20:26 schrieb Chiedo ***@***.***>:
Does that not happen for you?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
No worries @fk You were right. It was blown up some how. It's very weird though. Chrome shows it as being 1920 by 1600... Maybe something is wrong with how some of the meta is generated? Unsure... The actual image is the right size though. The browser just thinks it's the wrong size? |
(still AFK) Hmm, actually "Natural" should be the original size of the image :/ |
That makes sense! 381px is the maxWidth I need for that image. All the images on this page are generated dynamically. I worked around the issue for what its worth though |
Hi @bripkens! Interesting 😄 (as the original issue) … I will take a look in a few minutes and try to reproduce this locally. Til then: I believed that the resolution information embedded in the image is irrelevant for HiDPI/retina. Eager to understand how the resolution info affects dealing with retina/non-retina – could you maybe elaborate a little on that? |
Possibly related. In the example app, sharp is still at |
@fk: I don't know exactly how the browser uses the resolution fields, but at least OX's preview feature takes the resolution into account when displaying the image. It could be that image resizing needs to take this into account too, especially when generating the srcset definition on the image. This being set, I have actually never used srcset. Just suspecting. |
Updating sharp is definitely on the agenda, see #1920 (comment) for a little more info. I just finished compiling using-remark with one of the images saved at 144dpi (using Photoshop – |
FWIW I also noticed the |
OK, here we go … what I did was to take this source image from With the …the largest of them being 400px wide, which matches the desired behavior of "do not scale larger than the size of the original image source": Again, I hope I did things right to reproduce the problem you guys run into. However, if you inspect the "Properties" for |
Based on what I have read now, I believe the problem to be the following:
Based on this, I think that this is definitely a bug within Gatsby that needs to be resolved. Unfortunately, there is one obstacle to resolving it: We need a reliable way to identify image DPI. Anyone come across a Node module which can do this? A quick Google search didn't bring up any results :(. |
wow, parallel posting :) |
@bripkens Will do a "bonus round" with the DPI for the 400px source image set to 144, then get back to your comment. |
Which is exactly the issue imho. The image of |
Ah I see now where you're coming from.
As you state in 1., the browser does not care about the DPI/resolution information of the image, only about the actual pixel size. Am I correct to assume that in 2. when you write "supposed to be rendered" you are talking about sharp/gatsby-remark-images? |
Yes, right. You can try this, when available, by taking a screenshot from a retina screen on your Macbook. Opening that image within MacOS's preview app will look just fine as it respects the DPI. Open the image on the other hand within Google Chrome and it will look blurry as it will have twice the size. |
👍 Thank you! (and yes I'm aware of HiDPI screens/devices and on a MBP with a retina screen ;-) – I didn't know however that Preview.app takes the DPI into account when displaying images – nice!) I won't manage to post a thorough answer/reasoning during the next few hours, but basically my take is this:
Here are some good links on the topic of "the 72 DPI myth" …
… and some info on
If you peep the "sizes" section, you will find out that
On a 72dpi screen, the 740px wide image will be used: Now if I move that browser window to my HiDPI screen and load the page again, a 1480px wide image will be used: |
Hey of course! I'm totally unbiased about this and happy to discuss and learn new things/perspectives! 🤗
As previously stated I can totally follow you there, and given the context you provided I see the confusion this particular behavior results in. (It however is the intended behavior of Now let me take a look at that GitHub-flavored Markdown! 😄 |
Wheee, magic. :-D 🎉 Thanks for the heads up! I guess I never would have found out about that (probably because the images I upload here at Github always are larger than the comment container). Curiously asking (and not trying to use your potential answer as a base for argumenting against your usecase 😉): Do you know any other (Markdown-related or not) tools that provide that kind of functionality? |
Wrapping things up for tonight, I found one more (excellent, and english) resource hidden in a browser tab: https://gist.github.com/anotheruiguy/8626483 – also comes with a nice example page: http://www.anotheruiguy.com/hidpi-images/index.html And while I'm writing things again, I might as well add/clarify that:
|
Yeah that was my goal. Stop worrying about image size. Just add the biggest version you have and then size down from that with graphql queries. |
Nope, I don't. I tried it with Atlassian Confluence and Google docs real quickly: They don't automatically resize based on DPI (but of course they are WYSIWYG editors). This being said, I think that Github's solution to this is quite elegant and resolves a bunch of problems when one wants to provide retina ready images/screenshots and what @KyleAMathews has mentioned:
Defining retina-ready screenshots and images is often a pita. Github's markdown makes this effortless. I think this ticket alone is a good indicator that the current implementation does not yet fully satisfy the I believe that we understand this problem fairly well now and I outlined possible improvements to the respective packages in [1]. TL;DR; for that: Do not stretch images with |
Hey @bripkens, good to have you back! 😄 I gave all my blather 🤐 from yesterday a little more thought this morning (so more blather! ;-)), and almost immediately started thinking that the name
At some point I also remembered this name change, and in retrospect I find that to be a little unlucky. I can see how people would expect In regards to @bripkens suggested change, I must say I really like the "Do not stretch images with Implementation-wise, basically if the image is < plugin options maxWidth, the container Still haven't made up my mind about HiDPI though – just came as far as wondering about stuff like:
|
All good 🖐 Thanks for explaining the history around these packages. And yes, this is exactly how I interpret
They probably don't know about it and neither should they have to IMHO. This means to me a behavior similar to GitHub and MacOS's preview app.
I am not saying that 144dpi is high dpi. I think we should properly account for various DPIs by defining render sizes as Let me extend my arguments by asking an open question: Suppose I have an image |
In between things, so only a few random thoughts and things I missed earlier…
sharp provides
So the content authors are provided with these images, and the image authors take care of setting the "correct" DPI? Wondering about this also in the context of what you wrote earlier:
Ah damn it. I just re-read the quote another time, and now things are getting clear for me finally: So the people writing your docs are using retina Apple machines, and when they take screenshots with the built-in Apple tool, those get saved as 144dpi PNGs per default (you can change that to JPG, PDF and even GIF AFAIR) … Apparently they are using GitHub to author, which treats those images in a special way. Phew! Sorry it took me so long to get there. 😆
Ah, I forgot about that behavior. ;-) So when checking that yesterday, I gotta admit both of those began to creep me out a little. Not trying to be ignorant, and I might be wrong, but let me see if I still have my source images from last night … yes, at least a few! So your example image was a PNG at 548 × 264 at 144px/inch (and a DELL P2715Q color profile), this gets turned into an HTML-tag by GitHub magic:
|
Why no magic anymore if I upload a 144dpi JPG version of that image? :-P ;-) Now that I just got the gist of your workflow, I can see how Github supports exactly that – a Mac Retina screenshot; I guess that's quite a common thing to happen.😉 It also makes me wonder if they take care of other common HiDPI values(/filetype combinations) e. g. for Windows machines with HiDPI displays, too? |
@bripkins the whole point of gatsby-remark-images is to provide the right sized image to different devices with varying sizes and screen capabilities :-) look at all the screenshots in tutorials, etc. on different devices. I take screenshots on my Mac and then they get resized and then browsers use srcSet to pick the right image file to download. Check out this article https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images GitHub isn't doing anything this sophesticated. It just takes whatever image you give it and shows it to everyone which is far less efficient than what we do. |
Also, no special treatment for 145dpi PNGs (neither in Preview.app btw.): Anyway, moving on to the important question:
Good one! ❤️ 🗡 😃
To take advantage of I quickly gave "srcset sizes markdown" a shot on google:
Oh, also forgot those, and to explicitly mention |
@KyleAMathews In #1286 (comment) you wrote:
So I think the main Gatsby "markdown images" plugin should only copy images. What's your take on that? Do you see the plugin renaming in a different light given the discussion here? |
What's a use case for just copying? |
Also there's an open PR that adds support for copying that needs finished #1571 |
Not sure if I understand you correctly. "Just copying" as in "showing the unchanged image in the HTML rendered from Markdown". |
Ah yeah right, I remember that PR now! :D |
Not arguing against this. In fact, I see this to work perfectly fine in combination with my proposal. Only change would be the desired
This needs to happen via an HTML tag as the web is 72ppi. Forcing an image to be 144ppi requires you to use an image of size
That is a very interesting effect indeed! Wondering what their actual algorithm is? 🤔 So, to summarize for me: Either the behavior we see at work is intended or I just didn't articulate it well enough. Going forward, I see three options for me:
I can live with all of these options, but I figured that this behavior cannot be the desired behavior as it is confusing the heck out of everyone trying to use our new docs (when I say |
Use case for "just copying" is "default Markdown behavior". |
It's a crappy default :-) driven entirely by the lack of image processing capabilities in most markdown processing tool chains. |
@bripkens so is the problem that images are getting stretched really wide? |
@KyleAMathews Two things I guess, please correct me if wrong @bripkens:
|
Exactly right @KyleAMathews and @fk! Also, good summary @fk. To make this even clearer: A video to show the problem. I hope this will resolve any remaining confusion 😂. https://www.youtube.com/watch?v=EQYrXhXzmmU&feature=youtu.be |
👍 |
Plus an option to check for the image resolution and downsize the |
Shall we start by reopening this issue? And, how do we proceed? :) |
Eh this thread is super long. Could you create a new thread summarizing the current bug? Then someone (maybe you??? :-)) can create a PR that detects when the "natural width" of an image is less than the container width and changes CSS to constrain the width. |
Sounds great. Will do! Thank you for your patience :) |
When using the responsiveSizes function, in my opinion
maxWidth
shouldn't make an image that's 400px wide into an image that is 600px wide if a maxWidth of 600px is set.The highest maxWidth should be bound to the width of the image. I don't see the benefit of making an image larger.
Thoughts? If agreed, I'll add fixing this to my list.
The text was updated successfully, but these errors were encountered: