-
Notifications
You must be signed in to change notification settings - Fork 29
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
Aspect ratio container to resolve content layout shift (CLS) issues #218
Conversation
Breaking change (sort of) introduced in 3.4, so we workaround by duplicating the source file
Use nested selector for deeper components as one of the package updates broke the behavior somewhere
…esponsive-images into feature/aspect-ratio-cls
@riasvdv do you have opinion on this perhaps before I merge? |
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? |
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 |
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 |
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. |
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. |
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