[WIP] Block support: Add axial support for block gap #35301
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +313 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
7 tasks
stacimc
force-pushed
the
add/axial-support-to-gap-block-support
branch
from
2f6a463
to
96dc90b
Compare
andrewserong
reviewed
Oct 4, 2021
There was a problem hiding this comment.
Thanks for diving into this one @stacimc! The blockGap sure has wound up being more complicated than we'd expected 😅
Just left a comment about whether we want to continue rendering CSS variables inline rather than forcing the use of the gap value directly? I added a bit of context for why we previously rendered the CSS variables, but if we're exploring our options, that might not be the best way to go about it. Perhaps explicitly using gap values provides more direct control? Curious to hear what you think 😃
For simplicity I'm using horizontal => column, vertical => row, but the mapping back and forth between row/column and the normal axial terminology is definitely a pain point in this PR so far.
This is a tough one, too — the column and row wording is particular to row-gap and column-gap, but some blocks might wind up using margins or padding to calculate this instead, so we'll probably wind up having to do a bit of mapping between names in different implementations.
| } | ||
| } else { | ||
| if ( ! preg_match( '%[\\\(&=}]|/\*%', $gap_value ) ) { | ||
| $styles[] = sprintf( 'gap: %s', $gap_value ); |
There was a problem hiding this comment.
I know this is a WIP (and you're probably already well across much of this), but just wanted to raise that a couple of the reasons we went with rendering the CSS variable inline to the block instead of using the gap values directly were:
- Some blocks, e.g. Columns use margins between the individual elements. Using a CSS variable for the block support means that we don't force the blocks that opt-in to use any one particular approach for handling gap.
- For blocks that might have some more DOM nesting, we don't need to skip serialization if we use a CSS variable as the variable will flow down to the children.
However, a downside of setting the variable is that other styles that also use the variable will pick it up, which can cause an issue with margins. In the PR to opt-in the Columns block the workaround was to also opt-in to margin support so that it can be tweaked separately from the block spacing. This isn't really ideal though!
There was a problem hiding this comment.
Hm, there's definitely a lot of pros/cons to balance here. The issue with syncing the single gap variable with the individual row-gap and column-gap ones is a big headache that I'm still trying to reason through, and the inheritance issues are another big one.
Are you suggesting going back to using the gap values directly? Or specifically when supporting row/column gap separately?
There was a problem hiding this comment.
Hm, there's definitely a lot of pros/cons to balance here.
Totally!
Are you suggesting going back to using the gap values directly? Or specifically when supporting row/column gap separately?
Apologies, no, I'm not really suggesting one way or the other. I was mostly curious to explore the trade-offs and the potential gotchas we're going to run into. I still think there's a lot of benefit to using the CSS variables (and on balance, hopefully it'll give us a bit more flexibility), but either way we're going to have a few edge cases to wrangle. So mostly I'm curious to hear what you think from the exploration!
There was a problem hiding this comment.
I've been playing with @stacimc's idea a little bit in #35454
It's still hugely buggy, my latest revelation is that I think the multiple var approach seems to be the most flexible approach.
So far it means we can specify gap values for margin, e.g,
margin-left: var(--wp--style--block-column-gap, 2em);If we were to rely on a shorthand value, which was my first idea, we'd still need a new var to do the above.
It also mean that we're sending a bunch of vars to the frontend though:
Maybe there's a neater way to deal with all this, e.g., use blockGap: row col for the model but still generate the CSS vars (--wp--style--block-column-gap, --wp--style--block-row-gap) on the backend.
I'm still not certain.
7 tasks
18 tasks
|
Closing this one as #37736 got in. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #34529
Related: #34546
Based off the original approach in #32571
Description
Adds axial support to the existing block gap support. At the moment I'm doing this by adding two additional CSS variables, so:
A block can opt into axial gap support in
block.jsonwith a configuration like:For simplicity I'm using
horizontal=>column,vertical=>row, but the mapping back and forth between row/column and the normal axial terminology is definitely a pain point in this PR so far.Notes / TODO:
How has this been tested?
Update
theme.jsonsettings to enable blockGap:Pull in the changes from #34546 and #35303 . Buttons should have axial support enabled in
buttons/block.json:Play around with setting axial row/column gaps in
theme.jsonlike this:Play around with setting values in global styles, and from the Dimensions panel in the block editor itself.
Screenshots
TODO: Screen recordings
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).