Always pass layout to inner blocks #47477
Conversation
tellthemachines
requested review from
ellatrix,
geriux and
ajitbohra
as code owners
tellthemachines
added
the
[Feature] Layout
tellthemachines
requested review from
MaggieCabrera,
noisysocks,
andrewserong and
youknowriad
|
Size Change: -679 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Yay, this is exciting, great to see this tidy up! I probably won't get a chance to give this a detailed look before I wrap up for the day, so just added a couple of drive-by comments about the query deprecation that stood out to me while skim-reading. Apologies if I've missed details / context there, as I was only looking quite quickly! 🙂
| @@ -296,6 +296,80 @@ const v3 = { | |||
| }, | |||
| }; | |||
|
|
|||
| const deprecated = [ v3, v2, v1 ]; | |||
| const v4 = { | |||
There was a problem hiding this comment.
Can we add a fixture to match this added deprecation? (e.g. core__query__deprecated-4.html etc)
It might be a little tricky if the deprecation depends on an earlier version of useInnerBlocksProps.save() to make sure that we test the deprecation is being run correctly 🤔
There was a problem hiding this comment.
Thanks, I always forget about the fixtures 😅
| isEligible: ( { layout } ) => | ||
| ! layout || layout.inherit || layout.contentSize, |
There was a problem hiding this comment.
By including layout.contentSize in the eligibility list, what happens if a user sets a content size after it's migrated to a constrained layout? Will this match happen another time?
There was a problem hiding this comment.
That's a good question! I re-used the same logic from the Group block deprecation which seems to be working fine, but will double-check.
There was a problem hiding this comment.
Yeah, turns out it does run so I added an extra check to only run it if content size is set without constrained layout.
|
Flaky tests detected in f1acfb5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4050529877
|
|
Thanks for reviewing, folks! I've addressed the feedback so far and added a deprecation fixture so I think this is ready for another round. |
There was a problem hiding this comment.
Thanks for all the follow-up here @tellthemachines! I haven't finished working through testing all the blocks, but just thought I'd leave the review now as I think the change to the full/wide alignments for flow layout appears to reintroduce the issue that was resolved by #43502. Let me know if you need any other info there!
Otherwise, I like the logic change here, and it feels like it's honing in on a good structure for passing down layout in an intuitive way that results in blocks needing to do less work — it seems like this is nearly there!
| attributes = {}, | ||
| __unstableLayoutClassNames, | ||
| } = props; | ||
| const { layout = null } = attributes; |
There was a problem hiding this comment.
This might be super edge-casey, but I was wondering if we should check whether or not the block has opted-in to the layout support before passing down its layout attribute in the context? Because this will apply to all blocks, if a 3rd party block happens to use an attribute also named layout, then it will be passed down here, too, without a check for the layout block support.
There was a problem hiding this comment.
Good idea, let's do that!
| @@ -67,22 +67,13 @@ export default { | |||
| info: alignmentInfo[ alignment ], | |||
| } ) ); | |||
| } | |||
| const { contentSize, wideSize } = layout; | |||
There was a problem hiding this comment.
Just double-checking — is the idea that this can be removed now because of the layout object being passed down? From testing, the issue of the wide width post template showing up using content size appears to have returned in this PR. I think the flow layout might still need the logic here (or elsewhere), so that the alignwide and alignfull classnames can be injected.
With TwentyTwentyTwo theme applied, it looks like the post template is being rendered as wide width on the site frontend, but content width in the site editor:
| Editor (note 650px) | Site frontend (note 1000px) |
|---|---|
 |
 |
It looks like on the site frontend the post template block's output has both is-layout-flow and alignwide classes as expected, however in the site editor the alignwide class is not present.
There was a problem hiding this comment.
That code needs to be removed because it's causing align wide and full controls to always show, even if the parent block doesn't have a content width. The problem is that the layout we're extracting content and wide size from there is the full theme layout, not the block one, so it always includes those properties if they're set by the theme.
The specific problem those lines were fixing should now be fixed by the Query block deprecation. I only checked with the test markup from your PR, but will have a look at TT2 now.
| @@ -1,3 +1,3 @@ | |||
| <!-- wp:query --> | |||
| <!-- wp:query {"query":{"perPage":null,"pages":0,"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":true,"taxQuery":null,"parents":[]},"displayLayout":{"type":"list"}} --> | |||
There was a problem hiding this comment.
I couldn't quite work out why this change to serialized markup for the block with empty attributes is required. Is it because of the deprecation updating the attributes? In practice I don't think it really matters that this serializes the default values of the empty state of the block, but was just curious as to why that's happening now 🤔
There was a problem hiding this comment.
Oooh that's weird, not sure why it's happening 🤔
There was a problem hiding this comment.
I simplified the fixture a bit and removed some values I'd incorrectly copied from the previous deprecation, but no changes to this file. I guess it should be fine? 🤷
There was a problem hiding this comment.
Good idea simplifying the fixtures. For this one, I think it's just that what gets serialized for the block's default state has changed — I don't think it's really an issue though since it's just the block attribute's metadata for when it's in the placeholder state. I'll just CC @ntsekouras for visibility, but I don't think it'd be a blocker.
Funny, it's working for me. What happens when you paste the test markup into the code editor? Doesn't the Query block layout attribute get auto-updated with |
There was a problem hiding this comment.
Oh, interesting, yes if I cut the markup from TT2's home template so that there's a change to the site's markup and then paste it back in within the code editor view then the deprecation runs. However, for the template that ships with the theme, when opening the site editor, it doesn't look like the query block is updated immediately 🤔
This is the test markup from TT2:
Test markup from TT2's home template
<!-- wp:query {"queryId":0,"query":{"perPage":10,"pages":0,"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":true},"tagName":"main","displayLayout":{"type":"list"},"layout":{"inherit":true}} -->
<main class="wp-block-query"><!-- wp:post-template {"align":"wide"} -->
<!-- wp:group {"layout":{"inherit":true,"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:post-title {"isLink":true,"align":"wide","fontSize":"var(\u002d\u002dwp\u002d\u002dcustom\u002d\u002dtypography\u002d\u002dfont-size\u002d\u002dhuge, clamp(2.25rem, 4vw, 2.75rem))"} /-->
<!-- wp:post-featured-image {"isLink":true,"align":"wide","style":{"spacing":{"margin":{"top":"calc(1.75 * var(\u002d\u002dwp\u002d\u002dstyle\u002d\u002dblock-gap))"}}}} /-->
<!-- wp:columns {"align":"wide"} -->
<div class="wp-block-columns alignwide"><!-- wp:column {"width":"650px"} -->
<div class="wp-block-column" style="flex-basis:650px"><!-- wp:post-excerpt /-->
<!-- wp:post-date {"format":"F j, Y","isLink":true,"style":{"typography":{"fontStyle":"italic","fontWeight":"400"}},"fontSize":"small"} /--></div>
<!-- /wp:column -->
<!-- wp:column {"width":""} -->
<div class="wp-block-column"></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->
<!-- wp:spacer {"height":"112px"} -->
<div style="height:112px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer --></div>
<!-- /wp:group -->
<!-- /wp:post-template -->
<!-- wp:query-pagination {"paginationArrow":"arrow","align":"wide","layout":{"type":"flex","justifyContent":"space-between"}} -->
<!-- wp:query-pagination-previous {"fontSize":"small"} /-->
<!-- wp:query-pagination-numbers /-->
<!-- wp:query-pagination-next {"fontSize":"small"} /-->
<!-- /wp:query-pagination --></main>
<!-- /wp:query -->
So what I'm wondering:
- Is there some reason why the deprecation isn't initially running when the site editor opens the template, either in the deprecation, or in the site editor?
- If it's tricky to figure out, would it be worth retaining the wide / full alignment logic in the
layouts/flow.jsfile for backwards compatibility, or does it cause other issues keeping it in place? 🤔
| return { | ||
| ...attributes, | ||
| layout: { | ||
| ...layout, |
There was a problem hiding this comment.
Just noticed that if layout.inherit is true, then this still gets included in the object here. Should we add a inherit: undefined line here after ...layout?
There was a problem hiding this comment.
I don't think there's any harm in keeping it. We've been keeping it for the Group block deprecation, and on the whole I think the less we change the less scope there is for things to go wrong 😅
There was a problem hiding this comment.
The main reason I thought of removing it is that a truthy check for layout.inherit is included in the isEligible callbacks, so I was wondering if it'd be possible for a migrated block to be treated as though it hasn't been migrated if the inherit value sticks around 🤔
Totally agree with trying to change fewer things, though!
There was a problem hiding this comment.
We can also change the check to not run at all if type: "contstrained" is present.
There was a problem hiding this comment.
Why other blocks don't have a deprecation except for Query? It's weird to have an empty Query Loop with all the attributes set.. Also the migrate function should run only to this deprecation? Usually it's needed to also run for other migrations..
There was a problem hiding this comment.
The Query deprecation doesn't address this change, it should have been added as part of #42763. I added it here in an attempt to fix the issue reported in #43502, as the solution from that PR was causing other breakage.
Also the migrate function should run only to this deprecation? Usually it's needed to also run for other migrations...
I'm not sure I understand. Is there anything I should change here?
There was a problem hiding this comment.
I haven't tested, but this is from the docs:
It is important to note that if a deprecation’s save method does not produce a valid block then it is skipped completely, including its migrate method, even if isEligible would return true for the given attributes. This means that if you have several deprecations for a block and want to perform a new migration, like moving content to InnerBlocks, you may need to update the migrate methods in multiple deprecations in order for the required changes to be applied to all previous versions of the block.
You could test by copy/paste the other deprecated versions and see if there are errors.
There was a problem hiding this comment.
Oh, got it, thanks! I'll have a look.
Yeah, that's a very good question 😅 I'm noticing further that the deprecation only doesn't run if the template has no customizations, so there must be some check that skips brand new templates. That means we can solve the problem by updating the theme markup in Core, because any instance of the template already in use is likely to have some customization to it so should work. We should get to the bottom of the deprecation issue, but I'm hesitant to block this change for its sake, so suggest we do it as a follow-up task.
It does cause other issues - currently we have children of Group blocks with flow layout showing wide/full controls, which feels broken because those controls don't do anything if the parent is a flow block. |
Sounds good — if I go in to the template and make a change, e.g. adding a paragraph and then save and reload the site editor, then the migration happens. So it's fairly easy for a template to get to the correct state, it's just unfortunate that that isn't happening earlier 🤔
I always find these ones tricky where we need to choose between two things that might feel broken, and determine which is the less broken-feeling one. But in this case, and with the timeline on feature freeze for 6.2, I think I agree with the logic here 👍 I'll continue working through the other tests cases! |
There was a problem hiding this comment.
Alrighty, this is testing well for me, the only issues I ran into are the ones we've already discussed, and it seems to me that the trade-offs favour merging in the current state, as it's a big improvement to consolidate the logic surrounding passing down layout, and it's a great idea getting this in for 6.2.
I tested the following:
- The following blocks behave correctly in the editor and render correctly on the site frontend as far as I can tell:
- ✅ Buttons
- ✅ Comments Pagination
- ✅ Group
- ✅ Navigation
- ✅ Post Content
- ✅ Query
- ✅ Query Pagination
- ✅ Social Links
- ✅ Tested group block as wrapper for image block, and that wide/full alignement controls only appear for constrained layout type
- ❓ Checked that the scenario described in Layout: Re-instate alignwide and alignfull in flow layout get alignments #43502 is fixed — it is fixed once the deprecation is run, but in TT2 where the template does not have the latest markup, the deprecation only appears to run when the template is altered. I agree that it's not a blocker for this PR to land, but it'd be a good follow up bug to look into.
- ✅ Column block child image only has wide/full controls when constrained layout is set
- ✅ Row children width controls are working correctly
Nice work, LGTM! ✨
|
Thanks for the reviews and extensive testing @andrewserong 🎉 I'm going to merge this as is to unblock #47584 and hopefully also #45326, and start looking into the follow-ups. |
|
Hi @tellthemachines I no longer have wide/full alignment options on first level blocks since this has been merged, they do appear on child blocks though |
|
Hi @corentin-gautier, could you provide some more details to help debug?
Thanks! |
|
I'm using a custom theme which doesn't use the site editor so I only experienced this issue in the post editor, the issue doesn't happen in twenty twenty three EDIT: It does happen in twenty twenty three, but after a few seconds it goes back to normal Screen.Recording.2023-02-02.at.10.34.20.mov |
|
Hi @tellthemachines, @corentin-gautier, I am facing a similar problem. For some reason, it's only found in some themes, e.g. Upsidedown & Disco By Automatic, but there are more of these themes. You can test at this link. video.mp4 |
|
Hi @gyurmey2 , thanks for the report! I can confirm that on that test site, with Upsidedown theme, the wide/full controls don't appear for root blocks in the post editor. This happens because the Post Content block in the Single template doesn't have content width enabled:
You'll notice that if you disable the Gutenberg plugin, root blocks in the post editor now show the wide/full controls:
However, on the front end, those blocks set to wide/full are the same width as regular blocks:
So the change that this PR introduced brings consistency between editor and front end, in that if the Post Content doesn't set a content width for its children, they will always take up the full width of their container (which in this case is the Post Content block), so wide/full controls aren't available because they won't have any effect. @corentin-gautier I suspect something similar might be happening with your custom theme. The issue that your video captures in Twenty Twenty Three is slightly different, as that theme does have content width in its Post Content block. That layout change we can see in the video is due to a performance issue with loading post content styles in the post editor, which #45299 should fix. |
|
@tellthemachines, I think this report is also related to changes here - #47857. Edit: Confirmed by using |
|
@tellthemachines, thank you very much for the detailed explanation (now I understand why this is happening). Another problem I've been struggling with for a long time is the differences between the editor and the frontend. Could you help please? Same test link. |
|
@tellthemachines Thing is : I'm not using the template editor in my theme (I use classic php > twig files) which means I don't have a post content block because I don't have a Single Template. Which means this PR breaks the alignment on root blocks for classic themes ... It's exactly what's described in #47857 EDIT: Oh I see there's a PR for this already : #47961 |
|
@corentin-gautier I see, thanks for flagging this! It should be fixed with #47961. |
@gyurmey2 that is an actual bug! Looks like it has already been reported in #46724. I'll leave a comment over there to keep everything in the same place. |
What?
This enables the layout config of a parent block to always be passed to its inner blocks by default. It's one of the remaining steps towards the stabilisation of layout support 🙂
Why?
Addresses this comment, as well as miscellaneous unexpected behaviour around the ability to set wide/full alignments on child blocks.
It also removes a bunch of logic that wasn't actually doing anything from some of the core block edit functions 😅
How?
Main Features
BlockEditfunction;useInnerBlocksProps;Cleanup
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast