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

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

Closed
wants to merge 27 commits into from

Conversation

ncla
Copy link
Collaborator

@ncla ncla commented Apr 22, 2023

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 ncla marked this pull request as ready for review May 20, 2023 15:55
@ncla
Copy link
Collaborator Author

ncla commented Jun 2, 2023

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

@riasvdv
Copy link
Member

riasvdv commented Jun 2, 2023

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

src/Breakpoint.php Outdated Show resolved Hide resolved
@ncla
Copy link
Collaborator Author

ncla commented Jun 2, 2023

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
Copy link
Member

riasvdv commented Jun 2, 2023

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
Copy link
Collaborator Author

ncla commented Jul 30, 2023

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
Copy link

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 spatie-bot closed this Dec 1, 2023
@ncla ncla reopened this May 4, 2024
@spatie-bot
Copy link

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 spatie-bot closed this Sep 5, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants