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.

Sign up for GitHub

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

Jump to bottom

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

Closed
bweinzierl opened this issue Feb 8, 2021 · 6 comments · Fixed by #70
Closed

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

bweinzierl opened this issue Feb 8, 2021 · 6 comments · 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 manuelmeister mentioned this issue Sep 27, 2023
Merged
@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
@manuelmeister
Merge remote-tracking branch 'upstream/main' into bugfix/sitegeist#33
697604c
@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
@manuelmeister
Merge remote-tracking branch 'upstream/main' into bugfix/sitegeist#33
19e2256 
# 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?

mficzel added a commit to manuelmeister/Sitegeist.Kaleidoscope that referenced this issue Sep 27, 2024
@mficzel
Merge branch 'main' into bugfix/sitegeist#33
742fab2 
* main:
  TASK: Make BaseUri argument nullable in DummyImageSource constructor

# Conflicts:
#	Classes/Domain/DummyImageSource.php
mficzel added a commit that referenced this issue Sep 27, 2024
@mficzel
Merge pull request #70 from manuelmeister/bugfix/#33
1aa673e 
FEATURE: Only output existing images and allow upscaling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

FEATURE: Only output existing images and allow upscaling manuelmeister/Sitegeist.Kaleidoscope
2 participants
@manuelmeister @bweinzierl