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

Don't render srcset paths that are bigger than the original image #33

Open
bweinzierl opened this issue Feb 8, 2021 · 6 comments · May be fixed by #70
Open

Don't render srcset paths that are bigger than the original image #33

bweinzierl opened this issue Feb 8, 2021 · 6 comments · May be fixed by #70

Comments

@bweinzierl
Copy link

When the editor uses a small image and the fusion code has bigger sizes defined in the srcset property they are added to the srcset html attribute.
In our project we had strange effects because the browser was expecting an image with a specific width (as defined in srcset) but got an image with a different width than specified. The image in our case was not even upscaled. But even if it were it would make no sense to upscale an image.

This patch fixed the problem for us:

kaleidoscope-remove-upscaled-srcsets.patch.txt

@bweinzierl
Copy link
Author

We tweaked it a bit to prevent an empty srcset="" if the file is to small to provide even one srcset width.

If this is interesting for anybody i can provide a PR...

@manuelmeister
Copy link
Contributor

I like this idea. I'll create a PR with this.

@manuelmeister
Copy link
Contributor

@mficzel could there be a case where someone would want to upscale an image beyond the asset image size? Then it would be necessary to add an allowUpscaling flag in the srcset function.

@bweinzierl If we don't render srcset paths for generated images bigger than the original, then we would also need to implement this functionality for the 1x, 2x specifiers as well.

@manuelmeister
Copy link
Contributor

@bweinzierl Could you have a look at my PR #70? Would this solve your use case?

manuelmeister added a commit to manuelmeister/Sitegeist.Kaleidoscope that referenced this issue Oct 26, 2023
@bweinzierl
Copy link
Author

@manuelmeister I tested your PR and it solves our usecase perfectly.
@mficzel Can we merge this?

manuelmeister added a commit to manuelmeister/Sitegeist.Kaleidoscope that referenced this issue Jun 3, 2024
# Conflicts:
#	Resources/Private/Fusion/Prototypes/Picture.fusion
#	Resources/Private/Fusion/Prototypes/Source.fusion
@manuelmeister
Copy link
Contributor

@bweinzierl I removed the preserveAspect part to #77. Could you have a look as well there?

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 a pull request may close this issue.

2 participants