Quadrat: Add alignments for image cover and video #3816
Conversation
| @@ -193,8 +197,8 @@ | |||
| }, | |||
| "post-content": { | |||
| "padding": { | |||
| "left": "var(--wp--custom--margin--horizontal)", | |||
| "right": "var(--wp--custom--margin--horizontal)" | |||
| "left": "0px", | |||
There was a problem hiding this comment.
Need to look at the impact of this change
I'm not sure I agree. For example if I have a group or columns block with a different background color, should that extend to the full width of the page?
|
| @@ -129,6 +129,10 @@ | |||
| }, | |||
| "margin": { | |||
| "baseline": "10px", | |||
| "bodyHorizontal": "0px", | |||
| "bodyVertical": "0px", | |||
| "bodyHorizontalDesktop": "0px", | |||
There was a problem hiding this comment.
I'm concerned that by adding more custom variables within the theme that are media query specific, we are going down a similar path to alignments that won't be compatible with FSE.
There was a problem hiding this comment.
Do you think that these variables should be set by the Site Editor?
There was a problem hiding this comment.
I rather think they make sense as variables in a theme for re-use, but (IMO) doesn't HAVE to be surfaced to the user (and is unlikely to be a standard way). So they make sense in a child theme, just not BCB. I'm not sure that I'm keen for most of this to be in BCB; it all feels very theme-specific and something unlikely to be handled by gutenberg (thus not something to be handled by BCB).
There was a problem hiding this comment.
Since we have a standard method of describing padding perhaps it would be better to define this just like any other element. And to take a note from the button hover state refactor, perhaps could also express the mobile 'state' in the same way. i.e.
"custom": {
"alignment": {
"alignedMaxWidth": "50%"
},
"body": {
"spacing": {
"padding": {
"top": "0px",
"bottom": "0px",
"left": "0px",
"right": "0px"
}
},
"mobile": {
"spacing": {
"padding": {
"top": "0px",
"bottom": "0px",
"left": "0px",
"right": "0px"
}
}
}
},
It's more verbose but follows the convention of Gutenberg.
@beafialho what do you think? |
|
I'd say if a column block has a background color, the best solution is to extend it to the full width of the page. However, if it doesn't have a background color and the columns have text, the problem remains, so I think it should have the margins in this case.
@kjellr what do you think? |
|
That's right. The general rule is: No text should bump up against the edges of the screen. |
|
To account for that scenario does it make sense to just add a padding to the columns (and group) if it's wide width (background color or not?) |
|
It gets more complicated than that, because if for example there's a full-width image inside of a columns or group block, we do want it to extend to the edges. It's only text that we want to add padding to. |
Are you sure that's a good idea? @beafialho said it was better not to do that with the gallery block because of the spaces between the images: |
|
It's only text and the gallery block that we want to add padding to then. 😄 The priority is text though: when that bumps up against the edges it looks like a bug. When the gallery does, it's more of a design choice. |
|
|
||
| @include break-mobile { | ||
| padding: var(--wp--custom--margin--body-vertical-desktop) var(--wp--custom--margin--body-horizontal-desktop); | ||
| } |
There was a problem hiding this comment.
Leveraging and then redefining the variables here would allow for other places that use the body padding variables to do so only once (the value of the variable would change based on the breakpoint)
body {
padding-top: var(--wp--custom--body--padding--top);
padding-bottom: var(--wp--custom--body--padding--bottom);
padding-left: var(--wp--custom--body--padding--left);
padding-right: var(--wp--custom--body--padding--right);
@include break-mobile-only {
--wp--custom--body--padding--top: var(--wp--custom--body--mobile--padding--top);
--wp--custom--body--padding--bottom: var(--wp--custom--body--mobile--padding--bottom);
--wp--custom--body--padding--left: var(--wp--custom--body--mobile--padding--left);
--wp--custom--body--padding--right: var(--wp--custom--body--mobile--padding--right);
}
}
| margin: 0 calc(-1 * var(--wp--custom--margin--body-horizontal-desktop)); | ||
| width: calc(100% + var(--wp--custom--margin--body-horizontal-desktop)*2); | ||
| } |
There was a problem hiding this comment.
Redefining variables (as noted below in _body.scss) would eliminate the need for additional breakpoints like this. There would just be one calculation, the value of the variable changing depending on the viewport width.
|
Closing in favour of #3953 |
Changes proposed in this Pull Request:
In Quadrat we want a gap around the site, but we want image, cover and video blocks to extend into it:
All other blocks should have space around them.
Related issue(s):
#3747