-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Navigation block to render hooked inner blocks #57754
Update Navigation block to render hooked inner blocks #57754
Conversation
Nice, thanks again for working on this! I won't be able to review this more in-depth on Monday, so here's just a few high-level notes 😊
Right. This is going to be interesting. While we could in theory just run Chief among them is that per WordPress/wordpress-develop#5712, we use anchor blocks to store the information if a hooked block has been touched by the user. This is important in order to ensure that we stop inserting the hooked block if the user has persisted or dismissed it; in case of the Navigation block, this means e.g.
So that's going to be one of the core problems to solve here 😅 |
@ockham just an update:
This seems to work relatively well currently but when it comes to noting that a hooked block has been removed we may need to revisit this since we don't get any data about the parent block which will contain removed hooked blocks information. Additional to the above, I've made a note in the comment of the current code. At the moment there's some unnecessary serializing and deserializing happening because we're using the Perhaps there's scope to separate the two responsibilities of this function to |
Yeah, that's the major challenge here. While we normally (per WordPress/wordpress-develop#5712) use the So we need to find a way to carry over that meta information about ignored hooked blocks, or store it differently. I think I had an idea or two how we might pull this off. Since we're dealing with a The way to do this would be to basically take the mocked navigation block after we've run (Since WordPress/wordpress-develop#5712 was only merged rather recently, your dev environment needs to run WordPress
Good observation! There might be a number of different ways to tackle this (related: WordPress/wordpress-develop#5753), but TBH, I'd rather look into this only once we got hooked inner blocks insertion working 🙂 |
@ockham just an update on some changes to this PR where I've experimented with getting the
I've not done extensive testing to check this works in all cases but from some preliminary tests it works if you add the following attribute to your navigation block:
One thing I've not looked into yet is adding this attribute to the block when the user action of removing a hooked block is performed. As always, open to feedback / improvements / better ideas of approaches. |
Oh, this is coming together nicely! 😄 diff --git a/packages/block-library/src/loginout/block.json b/packages/block-library/src/loginout/block.json
index 59fceec596e..62e57c6b430 100644
--- a/packages/block-library/src/loginout/block.json
+++ b/packages/block-library/src/loginout/block.json
@@ -40,5 +40,8 @@
"fontSize": true
}
}
+ },
+ "blockHooks": {
+ "core/navigation": "lastChild"
}
} gives me the following: It's lovely to see how the block is present in the editor, and how it can be moved around within the Navigation block, just like we'd want it to! |
So the underlying paradigm for Block Hooks is to handle pretty much everything on the server side, and rely on the editor only implicitly. (This is originally motivated by the requirement that a hooked block needs to be rendered on the frontend even if the Site Editor has never been visited -- which means that the mechanism needs to work solely on the server side.) As a corollary, the Now if the template is saved, these attributes will be saved to the database as well! (Inside the corresponding We'll need some way to emulate this for the Navigation block. Currently, the It might be tricky though, since the block and its corresponding <!-- wp:navigation {"ref":4,"layout":{"type":"flex","justifyContent":"right","orientation":"horizontal"},"style":{"spacing":{"margin":{"top":"0"},"blockGap":"var:preset|spacing|20"},"layout":{"selfStretch":"fit","flexSize":null}}} /--> This makes it a lot harder to set the This is what makes me think that we might want to persist the
We'd probably need to make some minor adjustments on the editor side to keep that meta information around, and use it when updating the LMK if this makes sense to you, and if you're up for giving it a try! I realize it's maybe not the most obvious thing, so we can also get on a call some time this week to work on this together 😄 |
Thanks Bernie, just to summarise our Slack conversation and next steps here: Summary
Next steps:
|
Thoughts on this approach having worked on implementing it. Typical process of inserting a hooked blockTemplate parts from the filesystem: Template parts from the database: Proposed process of inserting a hooked inner block into the Navigation blockProposed approach: The problem: Somehow, we only want to store our hooked block in the Furthermore, currently a user can reset their |
The following conditions detailed here loads menu items from an 'unstable location'. I'm not sure how easy it would be to track One option would be to not hook them in at all, we can do this by checking if there is a navigation ID. |
Thanks a lot for clearly spelling out the current process, and the one we've discussed for the Navigation block!
There's one small aspect I feel it should clarify: When loaded on the frontend, the "Adds to (FWIW, we could theoretically even add something like a
Right; which is somewhat equivalent to what I pointed out above about the "typical process": We don't want to persist the
👍
Off the top of my head, I'd say that that's fine. Clearing customizations for a template or template part means basically deleting its corresponding If we ever feel the need, it'll be possible to introduce a close-enough counterpart for navigation menus, by deleting the corresponding |
BTW, when I suggested the following:
... I was (erroneously) assuming that we could update the My hope was to emulate the "typical process" by including I haven't entirely given up on that scenario (mostly in case this error proves to be deeper), but it might be tricky. I've verified that I can register a meta field to be included in the diff --git a/lib/compat/wordpress-6.5/navigation-block-hooks.php b/lib/compat/wordpress-6.5/navigation-block-hooks.php
index 48b5533e7a4..98c8276031c 100644
--- a/lib/compat/wordpress-6.5/navigation-block-hooks.php
+++ b/lib/compat/wordpress-6.5/navigation-block-hooks.php
@@ -113,3 +113,18 @@ function gutenberg_hook_first_last_children( $inner_blocks, $post_id = null ) {
// Note: If there are no hooked inner blocks, we return the original parsed blocks.
return isset( $parsed_anchor_block_with_hooked_blocks[0]['innerBlocks'] ) ? $parsed_anchor_block_with_hooked_blocks[0]['innerBlocks'] : $inner_blocks;
}
+
+function register_ignored_hooked_blocks_post_meta() {
+ add_post_type_support( 'wp_navigation', 'custom-fields' );
+ register_post_meta(
+ 'wp_navigation',
+ '_wp_ignored_hooked_blocks',
+ array(
+ 'show_in_rest' => true,
+ 'single' => true,
+ 'type' => 'string',
+ 'revisions_enabled' => true,
+ )
+ );
+}
+add_action( 'init', 'register_ignored_hooked_blocks_post_meta' ); (In practice, we'd want the To verify, I've manually set a post meta field via WP-CLI:
This is then indeed included in the entity fetched by However, there are still some pieces missing:
|
TBH this is something I wouldn't even tackle as part of this PR for the time being. I'm not too familiar with the Navigation block's fallback strategies, and I think it'll add quite a bit more complexity to figure out. My suggestion would be to keep this one focused on the "basic" scenario; whatever solution we find for that will hopefully carry over to pretty much anything that involves loading from a DB record. |
@ockham I'm ashamed to say this was likely the result of some rushed programming. I've committed a fix here a5434ff which should fix that specific bug so I don't think it goes "deeper" as we may have feared but it would be great if you could re-test. I've also updated the testing instructions on this PR to cover different methods of insertions & testing scenarios for my own sake. |
@ockham can you elaborate on a few of the below points for me please:
As far as I was aware you did that when you registered the meta with the
I am assuming by this you mean when requests (e.g. when saving an entity in the editor) you want to meta data to be sent along with the request which the controller will handle and save in the database? |
No worries! Glad it was an easy fix 😄
Perfect, thank you! |
No, what I meant wasn't about registering the meta 😅 It was about sending it to the client (i.e. the editor), without simultaneously persisting it to the DB. This would mimic the "typical process", where we also send the markup with the (Apologies for the confusion! Plus we might not actually need to do anything of the sort if your PR works fine as-is 😊)
Yeah, exactly! |
aeda9ea
to
bababb9
Compare
I've rebased, but we'll need to do some more tweaking 😕 |
|
||
if ( function_exists( 'get_hooked_blocks' ) ) { | ||
// Run Block Hooks algorithm to inject hooked blocks. | ||
$markup = gutenberg_insert_hooked_blocks_into_navigation_block( $blocks, $navigation_post ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to change this function name: Dynamic blocks PHP -- i.e. pretty much any packages/block-library/src/*/*.php
files -- cannot contain any gutenberg_
prefixed functions, as they are synced verbatim from GB to Core via npm package sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( function_exists( 'get_hooked_blocks' ) ) { | ||
// Run Block Hooks algorithm to inject hooked blocks. | ||
// We have to run it here because we need the post ID of the Navigation block to track ignored hooked blocks. | ||
$markup = gutenberg_insert_hooked_blocks_into_navigation_block( $fallback_blocks, $navigation_post ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed our functions following the convention and moved all code into |
Since all the code touched by this PR is inside Once that has happened, we should however update the |
@ockham are we sure using If I run WP 6.4.3 and Gutenberg trunk and test the following:
function register_navigation_last_child( $hooked_blocks, $position, $anchor_block, $context ) {
if ( $anchor_block === 'core/navigation' && $position === 'last_child' ) {
$hooked_blocks[] = 'core/search';
}
return $hooked_blocks;
}
add_filter( 'hooked_block_types', 'register_navigation_last_child', 10, 4 ); |
@tjcafferkey Ah, good spot! I wanted to use Using |
Fixes https://core.trac.wordpress.org/ticket/59743.
What?
In this PR we are hooking into two filters. One which allows us to update the API response based on post type (
wp_navigation
) to insert hooked blocks before it gets returned to the editor. The second allows us to insert additional hooked blocks to thecore/navigation
via theWP_Navigation_Block_Renderer
class before rendering happens on the frontend.Notes
This solution is very specific to the Navigation block and is not scalable when it comes to implementing it for other blocks. From my early understanding this is necessary since the Navigation block works slightly differently:
wp_posts
with apost_type
ofwp_navigation
.WP_Navigation_Block_Renderer
to render it on the server. I believe this is due to a number of reasons but one of them being that it contains logic to wrap certain inner blocks in<nav>
and<li>
tags.$context
value to the hook in addition to templates and patterns. You can differentiate this by checking that$context['blockName']
is set to determine if it's a block.Todo
first_child
andlast_child
blocks as inner blocks into the core Navigation block.Related: woocommerce/woocommerce#43498
Why?
How?
Testing Instructions
functions.php
block.json
file and retest starting from step 2.