Skip to content
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

Add FixedHeight and net7.0 #12

Merged
merged 4 commits into from
Jan 18, 2023
Merged

Add FixedHeight and net7.0 #12

merged 4 commits into from
Jan 18, 2023

Conversation

karlsvan
Copy link
Contributor

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
Copy link
Owner

ErikHen commented Jan 16, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

ErikHen commented Jan 17, 2023

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 karlsvan changed the title add StaticHeight and net7.0 Add FixedHeight and net7.0 Jan 17, 2023
@karlsvan
Copy link
Contributor Author

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 ErikHen merged commit 81f31b4 into ErikHen:main Jan 18, 2023
@ErikHen
Copy link
Owner

ErikHen commented Jan 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants