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

Quadrat: spacing #3818

Closed
jffng opened this issue May 10, 2021 · 6 comments
Closed

Quadrat: spacing #3818

jffng opened this issue May 10, 2021 · 6 comments
Assignees

Comments

@jffng
Copy link
Contributor

jffng commented May 10, 2021

In working on the custom logo + header spacing, I was struggling to implement the padding around the heading.

Currently we have two variables — custom.margin.horizontal and custom.margin.vertical — to use throughout theme.json and CSS. These spacing variables are baked pixel values.

A fixed value doesn't work well for container elements like wp-block-post-content and the header, where we may want the value to be responsive.

Some questions:

  • Do we want to overwrite the value of custom.margin using a media query? This seems dirty because it kind of goes against the principle of managing the values via theme.json. Currently we do not have a way of changes a variable value based on a media query. See Allow responsive styles via theme.json WordPress/gutenberg#27107 and I think there was another more recent issue related.
  • What if changed the value of the variable to be responsive itself ("intrinstic")? For example like we're doing with the typography: min( 5vw, 35px). If we wanted to go with that approach, what should those values be for Quadrat?
  • How would a case like this be handled, where the top padding is greater on mobile? I don't see how this design is achieved without a media query.
@MaggieCabrera
Copy link
Contributor

I definitely see the value of having media queries. One clear example that comes to mind is changing the order of items with flexbox or grid-template-areas for mobile. But I think those are very specific cases and I'm not sure what the best way to surface those in the editor is. I'm worried about the state of the mobile experience using the editor and at the same time, I can see how complex it can all get very fast.

For now, on Quadrat I think it's fair to use media queries when a value is not going to behave in an expected way (when it will be bigger on mobile than on desktop for example) and to try to use dynamic values/units when we can benefit from them like we do for fonts.

@MaggieCabrera MaggieCabrera removed this from the Quadrat v1 milestone May 11, 2021
@jffng
Copy link
Contributor Author

jffng commented May 11, 2021

I think it's fair to use media queries when a value is not going to behave in an expected way (when it will be bigger on mobile than on desktop for example) and to try to use dynamic values/units when we can benefit from them like we do for fonts.

So for something like custom.margin.horizontal — do you think we should update this value based on a media query?

@MaggieCabrera
Copy link
Contributor

So for something like custom.margin.horizontal — do you think we should update this value based on a media query?

If it's going to be always the same value throughout the site with very few exceptions, it makes sense to have a mobile custom variable in theme.json. If we are only targetting specific cases, I'd use CSS for those (such as the logo issue) and not surface that to the json file. The thing is, if it's always going to be the same value most of the time, then it's easier and cleaner to use min() max() than to write the extra css and variables.

@scruffian
Copy link
Member

I think there's some value in thinking about percentage units.

@jffng jffng self-assigned this Jul 6, 2021
@jffng
Copy link
Contributor Author

jffng commented Jul 6, 2021

We are implementing the site padding using responsive values already. Unless there is a specific area where we need the spacing to be different or something specific, I think this issue can be closed.

@pbking
Copy link
Contributor

pbking commented Jul 6, 2021

I agree that we have some pretty good examples of responsively spacing and second closing this item.

@jffng jffng closed this as completed Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants