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

Migrate spacer style from TT1 theme #109

Closed
wants to merge 1 commit into from
Closed

Conversation

pbking
Copy link
Collaborator

@pbking pbking commented Dec 3, 2020

The following is to replicate the styles found in /twentytwentyone/assets/sass/05-blocks/spacer/_style.scss

Where when in mobile width and the spacer has a style attribute assigned (assuringly for a height value)
force the height of the spacer to whatever the global spacing unit is set to be.

CSS variables cannot be used for media queries

(an example that does not work...)
@media only screen and (max-width: var(--wp--custom--spacing--mobile-breakpoint)) {

I don't know of a configurable way to manage breakpoints with theme.json

I attempted to construct a possible implementation of those settings in the theme.json
file and couldn't figure out how it might work so I have no suggestions there.

What are we trying to define and allow a user to configure... and what do we just want to be a part of the theme's style?

The fact that the spacer collapses at a breakpoint?
At what breakpoint it collapses?
What height it should collapse to?

Or is it just an aspect of the theme, the already configured spacing unit is enough variance.

If it just belongs as CSS in the theme, how do we normalize the media query breakpoints?

The following is to replicate the styles found in /twentytwentyone/assets/sass/05-blocks/spacer/_style.scss

Where when in mobile width and the spacer has a style attribute assigned (assumingly for a height value)
	force the height of the spacer to whatever the global spacing unit is set to be.

CSS variables cannot be used for media queries

(an example that does not work...)
@media only screen and (max-width: var(--wp--custom--spacing--mobile-breakpoint)) {

I don't know of a configurable way to manage breakpoints with theme.json

I attempted to construct a possible implementation of those settings in the theme.json
file and couldn't figure out how it might work...

What are we trying to define and allow a user to configure... and what do we just want to be a part of the theme's style?

The fact that the spacer collapses?
At what breakpoint it collapses?
What height it should collapse to?

Or is the below just an aspect of the theme, the already configured spacing unit is enough variance.
And if that is the case how do we normalize the breakpoints?
@pbking pbking marked this pull request as draft December 3, 2020 21:54
@kjellr
Copy link
Collaborator

kjellr commented Dec 4, 2020

I think we should just this rule out, honestly. I've found it to be a little weird personally — it unexpectedly overrides any specific setting the user has made. I believe it was added because some spacers seemed too large, but a better fix for that is just to use dynamic units for the spacer block (vh, vw, etc.). And that's already possible today.

@carolinan
Copy link
Collaborator

I also found this odd and did not realize this style was in there. It should respect the size the user set. We should consider removing it from Twenty Twenty-One.

@pbking
Copy link
Collaborator Author

pbking commented Dec 7, 2020

Closing this PR RE: comments above.

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

Successfully merging this pull request may close these issues.

3 participants