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

WidthCalculator needs some improvements when outputting small images #167

Closed
ncla opened this issue Sep 26, 2022 · 4 comments
Closed

WidthCalculator needs some improvements when outputting small images #167

ncla opened this issue Sep 26, 2022 · 4 comments

Comments

@ncla
Copy link
Collaborator

ncla commented Sep 26, 2022

There are two issues:

First, WidthCalculator curently uses glide:width value only to filter out widths that are larger than the specified value. But the initial widths that have been calculated have been based on the asset dimensions, which is not particularly good if the asset is in high resolution, while we need a really small resolution on front-end.

For example, I have 42x42px thumbnails for my gallery on mobile viewport, so I specify glide:width to be 126, and I expect decent width coverage for DPR of 1, 2 and 3 (42px, 84px and respectively 126px), but instead I only get 126px breakpoint because that is what is forced as only value, if no widths get calculated by WidthCalculator. A device with DPR of 1 should only need image with width 42px, but in this case it will request the only available image with 126px width. The steps multiplier 0.7 is using original asset dimensions for file size prediction, as such we end up with too large gaps between calculated widths.

The second matter is that because these gaps are too huge, you may end up where the browser determines that the blurry placeholder is the one that it will use. In my situation, if I do not cap the maximum width with glide:width, I get 32w placeholder and the next closest width is 672w. Because the picture container is 42px width, browser will pick the closest width, and I end up with blurry placeholders. While I can workaround this issue myself (either setting max width or outright disabling placeholders), novice users might find this confusing.

As such, some adjustments have to be made to WidthCalculator class.

@riasvdv
Copy link
Member

riasvdv commented Sep 26, 2022

Good idea, I'd be happy if it was a bit smarter, right now it's mostly based on a guesstimate of file size and if it's worth it to create a new image for the resulting file size. I guess we could add some "fixed" widths in there based on the original source or the requested max size (size & size / 2 & and size / 3 for example)

@ncla
Copy link
Collaborator Author

ncla commented Oct 5, 2022

I have been thinking about this for some time. I think I want to approach this in a way where WidthCalculator becomes something like DimensionCalculator that can be customized by developers. I can PR a fix that remedies this issue no problem, but I would like something more.

For example, I have had instances where I want fixed height for my responsive images, but unfortunately this addon does not handle height constraints at all, this addon follows the ratio instead. I have had a few places where page header is fixed height in a design, and having images generated with ratio was not efficient. Having ability to do whatever dimensions calculations I want would have helped here.

We could ship with some DefaultDimensionCalculator, which would cover like 98% of use cases for devs, but if devs wanted they could register their own DimensionCalculator class, that would receive asset, width, height and tag params and they can do whatever they want, and return array of dimensions when they are done.

Do they want super aggressive width calculator that covers all kinds of widths with an image every 20 pixels? Use SuperAggressiveDimensionsCalculator or create your own. 😄

What do you think of this?

@spatie-bot
Copy link

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.

@ncla
Copy link
Collaborator Author

ncla commented Feb 14, 2023

You can now fix this by binding your own dimension calculator introduced in v3 that would workaround this issue.You can read about it more in the changelog and in the #193 PR.

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

No branches or pull requests

3 participants