search

ErikHen / PictureRenderer.Optimizely

PictureRenderer for Optimizely CMS (former Episerver)
MIT License
13 stars 4 forks source link

Add FixedHeight and net7.0 #12

Closed karlsvan closed 1 year ago

karlsvan commented 1 year ago

Hi, took the liberty to create a pull request to this very handy code! We have a case where we need the height attribute to be equal across varying screens on top hero images. This is needed for Imagesharp to crop wide images on narrow, mobile screens, which otherwise turns very blurry because the height is stretched. If it wasn't for Optimizely and contentref we could have just added the attribute ourselves to the path, that's why I added it here, not the main PictureRenderer. We'll do as you like of course.

Also added net7.0 TFM just because Optimizely supports that now. Not needed or could be a separate PR.

ErikHen commented 1 year ago

Thanks for contributing! Targeting .net7 has been on my todo-list for a while :-). But I don't really understand the requirement. Do you want to show the image in a different aspect ratio for mobile screens?

karlsvan commented 1 year ago

No problem, took a while to figure it out myself and it's possible I've overlooked a simpler solution.

Yes, for hero images at the top of articles our design simply flexes the image container in from the sides, and keeps the height, thus changing aspect ratio from ~3.5 to ~1. This works well if we're running without any image resizing, css will crop the full size image for us. But the full size image is huge, and would greatly benefit from imageresizing and server side cropping on devices thats on a slow/metered network.

Without the height attibute the browser selects the smallest size in the srcset, imagesharp gives us a smaller version, but the height is smaller too because default behavior without set height is "keep aspect ratio" I guess. So to fit in our now square-ish container the height must be stretched on the client before its cropped and now its blurry :(

With both width and height attributes set, imagesharp with crop it before resizing and only the useful part of the image will be sent to the client without much adjustment there.

karlsvan commented 1 year ago

added to readme. If I'm not mistaken this would override AspectRatio because the height attribute would already exist in PictureUtils.AddHeightQuery in PictureRenderer?

ErikHen commented 1 year ago

Ok, I get it. And I think it's a valid use case. It would be awesome if you could add this to the core PictureRenderer instead. That way all can benefit from it, and there are unit tests in that solution. Maybe "FixedHeight" is a better naming than "StaticHeight"?

Thanks, I really appreciate it!

karlsvan commented 1 year ago

guess the doc and net7.0 is still relevant if/when you merge core PictureRenderer, so I'll let this PR live with that for now.

ErikHen commented 1 year ago

Thanks! Latest version is on it's way to the Optimizely nuget feed.