Post Template: Ensure layout classnames are not attached to inner li elements #41827

andrewserong · 2022-06-21T04:38:24Z

What?

Fixes #41026 by ensuring that the Layout block support for the Post Template block is only applied on the outer-most wrapper, and not on the first child of each instance of the inner query loop. This PR is an alternative to #41802.

Why?

Based on discussion in #41802, the Post Template block's inner li elements should not receive a Layout container classname. Within the Post Template block, there is a loop that iterates over the current query. For each instance of that loop, we need to output an li element with the block's inner blocks as the template.

Prior to this PR, the container Post Template block is rendered again for each of these inner instances, however this has an undesirable side-effect that the Post Template block's block supports are invoked an additional time for each of these instances of the loop, on the inner wrapper.

How?

When rendering the inner wrapper to contain the post template for each instance of the loop, set the blockname to a null name that does not correspond to any registered blocks. This ensures that no block supports are invoked on this inner wrapper element, effectively skipping all block supports for that inner wrapper, ensuring that Post Template block supports are only applied to the outer wrapper.

Testing Instructions

Add a Query Loop block to a post
In the Post Template, create some blocks to demonstrate what goes wrong if the Layout classname is erroneously added to a child block. For example, in the below screenshots, I've used a Columns block as the direct child of Post Template. In this case, it should be fairly obvious that the styling is incorrect, as the second Column will receive the margin-block-start gap of the default Layout type, adding an unexpected top margin on the second column.

Screenshots or screencast

In the Before screenshot below, note that the first child of the highlighted li element receives two container classnames (wp-container-12 and wp-container-11). In the After screenshot, it only receives the single wp-container-11 classname.

Before	After

…elements

ramonjd · 2022-06-23T02:00:10Z

packages/block-library/src/post-template/index.php

+ // This ensures that for the inner instances of the Post Template block, we do not render any block supports.
+ $block_instance['blockName'] = 'core/null';
+
+ // Render the inner blocks of the Post Template block, with `dynamic` set to `false` to ensure that no wrapper markup is included.


Suggested change

// Render the inner blocks of the Post Template block, with `dynamic` set to `false` to ensure that no wrapper markup is included.

// Render the inner blocks of the Post Template block with `dynamic` set to `false` to prevent calling

render_callback and ensure that no wrapper markup is included.

This is a good comment! Saves folks searching around for API docs. Would it be worth adding that 'dynamic' => false skips render_callback to explain the how and the what?

Oh, good suggestion! Yes, I think because of how unusual this block is, the more explanatory text, the better, to hopefully make it a little easier for anyone making future changes. I'll tweak the wording a little later today 🙂

Committed in c8e2814. Thanks!

ramonjd · 2022-06-23T02:04:35Z

packages/block-library/src/post-template/index.php

+
+ // Set the block name to one that does not correspond to an existing registered block.
+ // This ensures that for the inner instances of the Post Template block, we do not render any block supports.
+ $block_instance['blockName'] = 'core/null';


What would be the option for developers who would want block supports for core/post-template later?

Render them somewhere in the render callback?

The supports still get rendered on the outer wrapper (this function is called with core/post-template) so the block supports ~~are~~ should still apply there (I think!).

I'm not too sure about how we'd handle styling for the inner wrapper to be honest — I think the best bet for pattern design would be to have a Group block as the first child of the inner wrapper, and then folks can use block supports there. I was almost wondering if we could use a group block instead of core/null, but that felt like going down a rabbithole that might be a bit broader in scope than solving the current bug 🤔

Longer-term, I'd love for us to figure out how to reliably get block spacing and block supports working nicely for styling up different kinds of post template layouts, though!

ramonjd

This is working well for me, and it looks unintrusive

It'd be great to get a ✅ from @ntsekouras just in case I'm missing something!

Before

After

andrewserong · 2022-06-23T02:11:38Z

Thanks for reviewing @ramonjd, much appreciated! And yes, it'd be good to get another pair of eyes on this, too, as the hack of using a core/null block name to ensure we render something that doesn't correspond to an existing registered block might seem a bit unusual. Though I guess this is an unusual block 😄

Co-authored-by: ramonjd <[email protected]>

ramonjd · 2022-06-23T04:37:49Z

I think this PR will fix #41756 as well

andrewserong · 2022-06-24T05:53:00Z

@adamziel hope you don't mind the ping, but I wondered if you think this PR would be worth trying to get into WP 6.0.1? I've added the backport label, and I think @ramonjd and I believe this is ready to go, but wanted to get a second opinion from someone just in case, since it's a bit of an unusual fix 🙂

adamziel · 2022-06-24T08:51:56Z

@andrewserong thanks for the ping, sounds good to me!

andrewserong · 2022-06-27T00:26:51Z

Thanks Adam! Merging now 🙂

…elements (#41827) * Post Template: Ensure layout classnames are not attached to inner li elements * Fix linting issue * Clarify dynamic set to false comment. Co-authored-by: ramonjd <[email protected]>

adamziel · 2022-06-30T14:14:02Z

I just cherry-picked this PR to the wp/6.0 branch to get it included in the next release: d5c2f4f

andrewserong · 2022-06-30T22:50:05Z

Thanks Adam! 🙇

mburridge · 2022-09-14T12:11:11Z

Added the Needs Dev Note label in case this needs a dev note (either individual or as part of a "misc" dev note) for WP 6.1 release.

andrewserong · 2022-09-14T23:59:11Z

Thanks for following up — for this one, I'm not sure that it needs a dev note as it's a bug fix for the internal behaviour of the Post Template block, rather than a change to how the layout support works, which will be covered in other dev notes. Let me know if you think there is something that needs to be captured here, though!

mburridge · 2022-09-15T09:39:52Z

Thanks @andrewserong, I was going through a whole load of PRs so had to make a quick judgement call on each one. This one appeared to change the rendered markup so I added the Needs Dev Note label just in case. Feel free to remove it if you think no dev note is needed.

Post Template: Ensure layout classnames are not attached to inner li …

739bc6d

…elements

andrewserong added [Type] Bug An existing feature does not function as intended [Block] Post Template Affects the Post Template Block labels Jun 21, 2022

andrewserong requested review from ramonjd and ntsekouras June 21, 2022 04:38

andrewserong self-assigned this Jun 21, 2022

andrewserong requested a review from ajitbohra as a code owner June 21, 2022 04:38

andrewserong mentioned this pull request Jun 21, 2022

Post Template: Fix layout issue by ensuring layout classname is attached to wrapper li elements #41802

Closed

Fix linting issue

64b0ffd

andrewserong mentioned this pull request Jun 21, 2022

An extra container class is getting added to the first element of each Query Loop #41026

Closed

ramonjd reviewed Jun 23, 2022

View reviewed changes

ramonjd approved these changes Jun 23, 2022

View reviewed changes

Clarify dynamic set to false comment.

c8e2814

Co-authored-by: ramonjd <[email protected]>

ramonjd mentioned this pull request Jun 23, 2022

Query Loop Block: Unwanted Block Gap on Column #41756

Closed

andrewserong added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jun 24, 2022

andrewserong merged commit 9a761d3 into trunk Jun 27, 2022

andrewserong deleted the fix/post-template-block-inner-block-instances-by-skipping-block-supports branch June 27, 2022 00:27

github-actions bot added this to the Gutenberg 13.6 milestone Jun 27, 2022

adamziel removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jun 30, 2022

mburridge added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 14, 2022

andrewserong removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 15, 2022

andrewserong added the [Feature] Layout Layout block support, its UI controls, and style output. label Sep 29, 2022

ntsekouras mentioned this pull request Jan 17, 2023

[Block Editor - Post Template]: Fix layout styles #47221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post Template: Ensure layout classnames are not attached to inner li elements #41827

Post Template: Ensure layout classnames are not attached to inner li elements #41827

andrewserong commented Jun 21, 2022

ramonjd Jun 23, 2022

andrewserong Jun 23, 2022

andrewserong Jun 23, 2022

ramonjd Jun 23, 2022

andrewserong Jun 23, 2022 •

edited

Loading

ramonjd left a comment

andrewserong commented Jun 23, 2022

ramonjd commented Jun 23, 2022

andrewserong commented Jun 24, 2022

adamziel commented Jun 24, 2022

andrewserong commented Jun 27, 2022

adamziel commented Jun 30, 2022

andrewserong commented Jun 30, 2022

mburridge commented Sep 14, 2022

andrewserong commented Sep 14, 2022

mburridge commented Sep 15, 2022

	// Render the inner blocks of the Post Template block, with `dynamic` set to `false` to ensure that no wrapper markup is included.
	// Render the inner blocks of the Post Template block with `dynamic` set to `false` to prevent calling
	render_callback and ensure that no wrapper markup is included.

Post Template: Ensure layout classnames are not attached to inner li elements #41827

Post Template: Ensure layout classnames are not attached to inner li elements #41827

Conversation

andrewserong commented Jun 21, 2022

What?

Why?

How?

Testing Instructions

Screenshots or screencast

ramonjd Jun 23, 2022

Choose a reason for hiding this comment

andrewserong Jun 23, 2022

Choose a reason for hiding this comment

andrewserong Jun 23, 2022

Choose a reason for hiding this comment

ramonjd Jun 23, 2022

Choose a reason for hiding this comment

andrewserong Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

ramonjd left a comment

Choose a reason for hiding this comment

andrewserong commented Jun 23, 2022

ramonjd commented Jun 23, 2022

andrewserong commented Jun 24, 2022

adamziel commented Jun 24, 2022

andrewserong commented Jun 27, 2022

adamziel commented Jun 30, 2022

andrewserong commented Jun 30, 2022

mburridge commented Sep 14, 2022

andrewserong commented Sep 14, 2022

mburridge commented Sep 15, 2022

andrewserong Jun 23, 2022 •

edited

Loading