Add constrained/flow layout to Cover block. #45326
Conversation
tellthemachines
added
[Block] Cover
|
Size Change: +213 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice one, thanks for the quick follow-up!
Existing Cover blocks should not change their layouts. I haven't done any testing for this yet, but anticipate some tweaks may be needed.
Yes, it looks like we'll need some kind of intervention to work out how existing blocks should be handled. From testing with a Cover block on trunk set to the Center Center justification in the toolbar controls, there's a change in how the blocks are laid out. Here's a before and after:
| Trunk | The same post loaded with this branch |
|---|---|
 |
 |
Also, it looks like the justification controls are exposed in the layout controls in the sidebar, which are different and conflict slightly with the ones the Cover block provides in the toolbar. E.g. justify left and right only work if the Toolbar control is set to Center Center:
Should the justification controls be hidden for the moment, or alternately, should they play nicely with the Cover block's ad hoc controls I wonder 🤔
| @@ -192,6 +198,7 @@ function CoverEdit( { | |||
| templateInsertUpdatesSelection: true, | |||
| allowedBlocks, | |||
| templateLock, | |||
| __experimentalLayout: usedLayout, | |||
There was a problem hiding this comment.
Is there a way to inject this automatically in the block support instead? It seems this should be done automatically for blocks that have the "layout" block support enabled no?
There was a problem hiding this comment.
It should, and this is one of the things we need to tackle before stabilising layout. Currently, the alignment controls of the child blocks depend on the layout settings being passed explicitly through innerBlocks. We have an open issue for it: #43731
I thought I'd go ahead with adding layout to Cover as it's one of the back-compat features requested by folks adding theme.json to their classic themes (so that Cover children can use the theme content width - see #33374 for more context). Once we have worked out a way to do this automatically, we can go back and remove the logic from existing blocks; we already pass layout to innerBlocks in most blocks that use it.
tellthemachines
force-pushed
the
add/layout-to-cover
branch
from
3722eb7
to
ca7a319
Compare
|
Thanks for reviewing and testing, folks!
I think that's an inevitable consequence of making "constrained" the default, so I've made it default to "flow" for now. This won't be great for the classic theme folks wanting to experiment with theme.json though - the idea of having "constrained" as default was to make that transition process more seamless.
Unfortunately Cover block matrix and the layout justification controls have completely different implementations, with the matrix styles coming from the block library stylesheet, so there's no way of making them work together unless we want to try re-implementing matrix as a layout-type control 😅 so I've hidden the justification controls for now. |
|
Thought I'd try what we're doing with the Group block and add constrained layout as a default block variation. This should mean that existing blocks will get flow layout applied but any new blocks added will be constrained. This seems like a good intermediate solution, and it's the same we adopted for Group. What does everyone think? |
|
Flaky tests detected in 543568c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4139187243
|
There was a problem hiding this comment.
Thanks for your work @tellthemachines!
I'd feel more comfortable if this could get more testing before landing today and therefore in 6.2. I haven't tested thoroughly but there are visible style changes in existing Cover blocks.
I have an example screenshot below that shows the margins are different for innerBlocks.
Screen.Recording.2023-02-01.at.11.32.32.AM.mov
| attributes: { layout: { type: 'constrained' } }, | ||
| isDefault: true, | ||
| scope: [ 'block', 'inserter', 'transform' ], | ||
| isActive: ( blockAttributes ) => |
There was a problem hiding this comment.
I don't think we need isActive and scope:block|transform here, since we don't have any other variations..
|
Thanks for taking the time to review and test this @ntsekouras ! I thought with the latest commit existing blocks weren't changing anymore, but something must have escaped me. If you have a moment, could you share the markup for those test blocks where you see changes? |
What I did was just create a Cover block(copy many times 😄 ) and then set different content position to each. Then test that content with your PR.
|
|
Oh, I think I see what you mean @ntsekouras. Currently, Cover block children are inheriting user agent margin styles unless they have any block- or global-level margins set. With the addition of layout, they instead get the layout margins applied, which means the first child has no margin and its siblings have a margin-top of whatever spacing value is set by the theme (or by core if the theme doesn't set any). I'd be inclined to consider this an improvement instead of a regression 😅 because, although technically there is a change, that change makes spacing inside Cover consistent with the rest of the site, e.g. post content already has those spacing rules applied. |
There was a problem hiding this comment.
I'd be inclined to consider this an improvement instead of a regression
This is a really interesting point, I think I'm inclined to agree that by using the layout support's margin-block-start: 0 rules this is effectively a bug fix in that the positioning controls of the Cover block now work more consistently. In a test post where I tried out each of the permutations, I found it noticeable that in the center positions, with this PR applied, elements are correctly centred where they weren't before due to the paragraph's user agent margin styles, as you mention. E.g. here's a before / after of vertically centered, align right — note that in the before version, the browser default top margin of the paragraph block causes the blocks to sit too far down:
Similarly, it's quite noticeable when looking at top alignment:
Whereas, using the same pattern, the issue isn't noticeable in the bottom alignment, because the Buttons block is the lowest block visually, so doesn't have a browser default margin (it would be noticeable if the paragraph block were the lowest visually):
So, I think there's a compelling argument that by introducing the layout support to this block, it either fixes a bug in the behaviour of the alignment matrix, or is an enhancement to the alignment matrix, in that the positioning is now much more consistent in what it does — and likely closer to a user's expectations for how it would work.
The main problem, then, is that it is a breaking change for sites, so whether or not it is an improvement might be subjective from the perspective of a site owner. An argument in favour of landing this change even though it is a change from what's on trunk is that it looks like the change only affects sites that already opt-in to using blockGap (either directly or via appearance tools). For example, if I switch appearanceTools to false or blockGap to null, then I can get back the extra space above the paragraph:
On balance, I think the changes in this PR are largely positive (good default constrained layout setting, fixed behaviour of alignment matrix for sites that use blockGap), so I'd be in favour of going with this. Would it be worth gathering more feedback from folks / a design review, to confirm the approach? From my perspective it's looking good from both a functionality and code perspective.
Great work so far! 🎉
|
Just one more thought, since we're opting in to the layout support, should we add support for
|
I agree with the part that this is an improvement, but unfortunately is also a regression 😅 We can get some more feedback here.. My opinion would be that we can land this(from the style changes perspective), but probably not in 6.2 at this point. |
|
Thanks for the extensive testing @andrewserong !
Yes, great idea! I'll give it a go.
Oh, definitely not in 6.2 now that we're in Beta. Let's merge it in the plugin and see how it goes for a while first! Also cc. @WordPress/gutenberg-design for feedback, and some themes folks perhaps? @carolinan @jffng @mikachan |
|
Seems to be working as advertised. It wasn't clear to me why I can set a wide width on children like images but not paragraphs, but that seems to be a separate thing as it occurs with Group blocks too. Other than that, the only thing that tripped me up was this variation button:
I don't think we need it? |
There was a problem hiding this comment.
This is testing well for me!
I agree that using the layout margins/spacing is an improvement, as this improves the Cover block positioning controls. But I also understand the change in spacing is a regression for existing users.
I also like the idea of supporting blockGap here.
I'm in favour of including this in the plugin for now and gathering more feedback that way.
|
This is probably a side comment but... To me this is one setting that needs to be re-thought. I see the result when toggling it on in the Group block when I change the Content and Wide fields but I am still not sure how it in general affects the layout of other elements outside of it. I am hesitant in adding the above setting to the Cover block as it just feels too vague for me and difficult to really understand. When I initially began looking at this PR I thought it was related to the "Change content position". I noticed I was wrong it was more to do with the width of the content inside the Cover block then the "Change content position" setting. Having a clearly defined "Change content position" would help a lot. Perhaps finding a way to simplify the "inner blocks use content width" toggle setting would help a lot as well. Finding a way that with a quick glance this specific setting becomes easier to understand. |
|
Thanks for the feedback everyone!
Yeah, I think the logic is that blocks considered "inline" like paragraphs don't have block alignment settings. It's always been like that, though perhaps it should be revisited at some point!
Ah yes, that was a bug 😅 Regarding the addition of block spacing controls, it proved more complicated than expected because spacing styles output wasn't taking the multi-wrapper markup structure of Cover block into account. Because I had to jump through a few hoops to enable it, I decided to do so in a separate PR: #47952. (It's currently pointing to this one so it can be properly tested with Cover block) |
tellthemachines
force-pushed
the
add/layout-to-cover
branch
from
543568c
to
6082aee
Compare
|
Nice, Something to consider: while layout is toggled off (fullwidth inner blocks), then the matrix position control is a bit confusing. You can only have top, middle, bottom positioning. CleanShot.2023-03-06.at.14.53.55.mp4 |
| @@ -13,6 +13,7 @@ import edit from './edit'; | |||
| import metadata from './block.json'; | |||
| import save from './save'; | |||
| import transforms from './transforms'; | |||
| import variations from './variations'; | |||
There was a problem hiding this comment.
Do we need variations? I don't think so, yea?
There was a problem hiding this comment.
We do need them unfortunately. The variation here is used as a workaround to apply constrained layout to all newly added blocks, without forcing it on existing blocks, which could potentially break a lot of websites.
(Making new blocks constrained is meant to match the behaviour of Group blocks, and also mitigate the experience for hybrid themes that suddenly lose all their legacy layout styles when they add a theme.json)
Good point! Might be good to get some design input on that cc. @WordPress/gutenberg-design |
Looking at this a bit closer, the behaviour with layout toggled off matches what we have in trunk, as currently the children of Cover always take up its full width. Likewise, on this branch, with layout off it's still possible to move the content around as long as it doesn't take up the full block width. For example:
So it should be fine to keep this behaviour as is! |
Yea, good point. |
Why?
Closes #43140.
Adds support for constrained (as default) and flow layout to Cover block.
How?
Now that #44600 is merged, layout classnames are applied to the block inner wrapper by default. This means that layout support will work in Cover block in the same way as it does in e.g. Group (but without the flex variations for now).
Testing Instructions
TODO:
Existing Cover blocks should not change their layouts. I haven't done any testing for this yet, but anticipate some tweaks may be needed. I don't think a deprecation should be required because we're only adding functionality, not changing anything existing.
Screenshots or screencast