search

spatie / statamic-responsive-images

Responsive images for Statamic 3
MIT License
99 stars 29 forks source link

Aspect ratio container to resolve content layout shift (CLS) issues #218

Closed ncla closed 4 weeks ago

ncla commented 1 year ago

Would fix #217 #216. Wrote this over one evening, roughly it works. Uses aspect ratio CSS hack and we add CSS now to the template.

This would be a breaking change

ncla commented 1 year ago

@riasvdv do you have opinion on this perhaps before I merge?

riasvdv commented 1 year ago

Is this something you can put behind a config? Default to disabled now so it's not a breaking change?

ncla commented 1 year ago

Is this something you can put behind a config? Default to disabled now so it's not a breaking change?

Sure technically we could. Do you have an opinion against releasing another breaking change version? Less major breaking releases is better, in your opinion?

riasvdv commented 1 year ago

Breaking changes are manual updates for people, if we can avoid them I’d rather avoid it, I can also imagine not everyone wanting this functionality, so a config flag feels better as well

we could set it to be enabled by default in the next major version

ncla commented 1 year ago

After some break, I have made these changes opt-in either through a hidden config setting or you can just simply copy paste the new template. The methods that are used by old template have not been changed and do not need to be changed, so both versions of the template will work. As discussed, the new template will become default with next major release (lets time it with next major Statamic version) to allow for breaking change (just in case for those projects that had not published the templates to their project).

The only remaining thing I have doubts about is if I should redo the CSS to use native aspect-ratio property instead. The padding-bottom CSS hack has been kinda engraved into my brain as the go-to way to do containers that maintain aspect ratio, but I am wondering if 91% browser support is enough and has enough time passed that padding-bottom trick is dated. https://caniuse.com/?search=aspect

spatie-bot commented 10 months ago

Dear contributor,

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

spatie-bot commented 4 weeks ago

Dear contributor,

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