-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
ImageMedium#derivatives now works with image filters #1107
ImageMedium#derivatives now works with image filters #1107
Conversation
Previously, using ImageMedium->derivatives would not work well in combination with image filters or the other method of generating srcset variants of images (by appending eg. "@2x" to their filenames). This commit hopefully fixes that.
The biggest alternative will now become the base medium, and alternatives will be filled out as necessary in a descending manner.
Otherwise we get some funky results, with the possibility of two different images being rendered between the full-width srcset version and the original src version.
When using naming conventions to generate image alternatives, this patch would previously generate a “@1x” alternative if one didn’t exist. That’s no longer the case
When an image only has an alternative medium - ie. the only version of the image ends in eg. "@3x", then we construct the missing alternatives automatically. Previously, we would only do this down till "@2x", meaning that the image that would have been the base medium, had the image been manually resized, wasn't created. This has now been fixed.
@rhukster the issue you described in #1035 (comment) seems to have related to 07c9591. I fixed that by always appending an |
Ok, definitely working better now. However, I don't agree with one change you have made. Before when using regular retina images, the default In your PR branch, this now uses the largest image, eg
All the examples I have ever seen on src/srcset usages shows the src containing the smallest image available. That was our previous behavior, I think it should be the same going forward. Of course, the same issues applies to derivatives, the largest image is being used in src there too. |
Oh one other thing I noticed. Your PR does fix filters in derivatives which is great, but you MUST put the filter after the derivative. It would be nice if derivatives are always handled first (like automatic @2x/@1x generated images) then removed from the list of actions. This way you could put filters first if you wished, and it would still work. |
One more potentially useful feature, perhaps you can use the |
The reason that the biggest image is now the base medium (if one wasnt present to begin with) is to cover scenarios where the content provides just an alternative medium (eg an I totally agree that it would be much more intuitive to be able to call Seems like the |
I feel like derivatives is a much more advanced and less used feature than the simple @2x method. That is used by 'most' people looking for quick retina support. So with that said, a change to add filters and other improvements support to the much less used derivatives functionality, should not change how @2x style retina works. It may not seem like a big change to you, but for older browsers without srcset support, a site could potentially be sending HUGE images to these clients. Another example is with site testing tools that don't check srcset, now are going to spit out errors about huge, un-optimized images.. I really don't see a benefit for this anyway. The whole point of the larger image is for retina support. If you want retina support, then your device is pretty new, and will very likely support srcset. If you browser supports srcset, then you will get the the appropriate image anyway. Why put the biggest image in src? makes absolutely no sense to me. |
Maybe you misunderstood my previous comment. What I proposed was to revert the behavior to use a scaled down image as the base medium when there's no base medium available on disk. The reason for the current solution in here was to cover for situations where theme developers call So maybe a better solution (like I suggested above) would be to change the logic of the So basically - always make the smallest image the base medium, ie. the one rendered in |
Ah ok, i misread (was early!), that sounds like a good solution. :) |
Cool :) Will try to churn out those changes as soon as possible |
When an image lacks a base medium on disk (eg. the only existing image is an @2x version), then we make a scaled down version the base medium, which ensures that the smaller version is served up in the src attribute in the HTML. Also, don't reset the image alternatives when calling ImageMedium#derivatives, instead only generate the image alternatives that are missing.
@rhukster I pushed some more changes that should hopefully deal with the things we discussed yesterday. Something to be aware of is that the
|
Good solution! I implemented and pushed it. Anything else we need to address before this is ready to merge? |
It's close but it's not quite right...
The |
@rhukster were all of those alternatives generated using |
All generate from a single 2500px wide image called yosemite.jpg. |
@rhukster I tested again, and I think it actually does work as expected for me. I'm using this Twig markup {{ page.media['world.jpg'].derivatives(500, 3500, 500).html() }} The source image is <img
src="/user/pages/02.test/world.jpg"
srcset="/images/f/4/6/9/8/f4698c102cf59cf97b5227eacc60639cf9e6aa8a-world2x.jpeg 500w,
/images/d/5/1/3/8/d51387c3e66cffe68c8b7c6f41e272372d65de82-world3x.jpeg 1000w,
/images/8/b/2/4/9/8b2494ee697a9c66995d6a7f52b7374f3c913ef3-world4x.jpeg 1500w,
/images/f/c/5/7/4/fc5749067f023c986b9cfc05a741b748a6285bcf-world6x.jpeg 2000w,
/user/pages/02.test/world.jpg 2500w"
sizes="100vw" /> It's strange that the primary source in your example directs to a cached image and contains |
Actually I did have it as
and get:
it's still not right though, first it probably makes more sense to just use the width in the derivatives rather than the
|
If we're skipping the I realize that I probably was a bit unclear in my previous comment, but I didn't intend for the smallest alternative to be returned If we reset each alternative when calling Maybe I'm missing a much simpler solution to this, but I'm not sure it'll be intuitive or elegant to write special logic that returns the smallest derivative when calling Btw, what's the purpose of resetting the image and its alternatives when calling |
Instead of example2x.jpeg, we now have eg. example1280w.jpeg
It's been a long road but I think we have reached a good end result! Thanks for your hard work. |
Cool, thanks! :) Still a bit interested in the |
This is a follow up to #1035, I had to close that PR since I forgot to point it to a feature branch.
Original description:
My original motivation for writing this PR was the problem described in #1016, where using the derivatives method on an image in a Twig template wouldn't work well in combination with the other method of producing responsive images in Grav - appending a pixel density indicator to the image's filename (eg. "@2x").
The underlying problem here also meant that image filters would not work in combination with image derivatives. They would work with image alternatives, which is what was generated from appending "@2x" to the filename of the image, but it wouldn't work with derivatives.
This commit fixes both the first and the second problem by making it so that the ImageMedium#derivatives method doesn't add image variants to a separate array, but rather to the same array as it would have if the filename method was used.
I've made some line comments to highlight a few things in the code as well!