Navigation: Allow Search block to be added alongside links #22656
Conversation
noisysocks
added
[Type] Technical Prototype
|
Size Change: +560 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
I have tested this and it works quite well. I like the approach as it is using the full extent of our current data structures and data flow, without any new additions to them, which is great! I support the proposal to make this opt in with Opting in will also solve the problem of emptu
I stumbled upon this while exploring additional blocks as children of |
|
cc @TimothyBJacobs and @spacedmonkey as it touches the rest api endpoint |
I support what @draganescu said about opt-in capabilities. I also don't necessarily think this is a bad thing - conceptually it adds a new type of data which breaks BC a little bit, let's make it clear, easily debuggable, and apparent in the changelog. Concealing it by keeping |
@noisysocks My understanding is that the way it currently work is this:
<InnerBlocks
allowedBlocks={ [
'core/navigation-link',
'core/search',
] }
/>
const allowedChildBlocks = intersection(allowedBlocks, childBlocks);
render( allowedChildBlocks.length ? allowedChildBlocks : allowedBlocks );This means that declaring a |
|
Maybe we could separate "supported children" from "supported parents" like this: // navigation/block.json
{
"name": "core/navigation",
// Restrict possible children to the following ones only
"children": [
"core/navigation-link",
"core/search"
]
}
// navigation-link/block.json
{
"name": "core/navigation-link",
// Restrict possible parents to the following ones only
"parent": [
"core/navigation"
]
}And then make it extensible with something like: // third-party-search/block.json
{
"name": "third-party/search",
"extend": {
"core/navigation": {
"children": [
"third-party/search"
]
}
}
}Although this would break BC and also involve changing the core heavily so maybe there's a simpler way? |
This is definitely how it used to work (#6753). Do you know why it was changed and if there's an alternative, @youknowriad? |
The problem with making it opt-in is that it means that the vast majority of themes and therefore the vast majority users will not gain access to blocks in this new context. If we go this route we will want to heavily publicise and document how theme authors can easily adapt their exiting themes to support blocks in navigation. I wonder, though, if there's a way to make the new markup more compatible with existing theme stylesheets? I'm not sure if it's an impossible problem or if I'm just fresh out of ideas 😀 What do you think, @mtias? |
|
This approach looks good to me, we need to get test coverage on the new endpoint field. |
There was a problem hiding this comment.
I suspect a great many themes will not work nicely with something other than an element inside the
.
What might go wrong, apart from minor styling breakage? It's not looking too bad with the default themes. Using add_theme_support() sounds good though.
We might also need to get rid of some default search block styles: (but that shouldn't be a matter for this PR)
|
Thanks for the feedback everyone! I've added a summary of next steps as I see them to the PR description. |
|
How should this behave in the navigator? |
I'd expect it to just render like the main navigator (it's own block type with icon). Did you have any concerns in mind? |
Not as much concerns as I wanted to get on the same page. So it would be just a regular navigator item labeled e.g. "Search"? |
I'm working on #23231 which will fix this. |
Indeed:
|
noisysocks
force-pushed
the
try/search-in-navigation
branch
from
f01a635 to
3d04332
Compare
|
The inserter that appears when adding a new block to the Navigation has some inconsistent spacing, and perhaps a redundant "Navigation" icon+label:
I'd expect it to look something more like this:
-- In general, the Search block leaves a lot to be desired; Its very restrictive in its current state. I realize this is unlikely to be resolved in this PR, but we do need to address it at some point. -- In the editor I have two navigations, both with a Search block, and it mostly looks as I would expect:
But when I preview I just see a general "Search" label that doesn't do anything:
|
|
Oh, and the search box on the inserter seems entirely useless. I get why its there, and I didn't even question it at first, but now I realize it doesn't really offer anything helpful. |
noisysocks
force-pushed
the
try/search-in-navigation
branch
from
3d04332 to
60bbbb8
Compare
|
I've rebased this and it's ready for review! 🙏 Worth noting that #22789 means that the inserter flow is now a little nicer.
|
Adds support for nav menu items with `type` set to `'html'`. These are nav menu items that can contain HTML or block markup which is rendered in place of the link. This allows the Navigation screen to use `'html`' menu items to support the Search block alongside links.
adamziel
force-pushed
the
try/search-in-navigation
branch
from
6dd653a to
c8d3bcc
Compare
| if ( menuItem.type === 'html' ) { | ||
| const parsedBlocks = parse( menuItem.content.raw ); | ||
|
|
||
| if ( parsedBlocks.length !== 1 ) { |
There was a problem hiding this comment.
This will kick in once a block plugin gets disabled. I don't think there's a good way of addressing it, but just wanted to double check - is this behavior consistent with the post editor?
There was a problem hiding this comment.
If a block plugin is disabled then parse() will return an array containing one core/missing block. The body of this if() won't be executed and so a core/missing is what will be used for the menu item. This matches the behaviour of the post editor.
The if() here is to guard against the case where, for whatever reason, there is 0 or >1 blocks in the menu item's HTML. I'm not really sure what to do in this case—it's undefined behaviour and probably indicates a bug somewhere. Perhaps failing loudly would be better?
There was a problem hiding this comment.
I put it on my todo list to come back to this as will need to think on it further.
| if ( block.name === 'core/navigation-link' ) { | ||
| attributes = { | ||
| type: 'custom', | ||
| title: block.attributes?.label, | ||
| original_title: '', | ||
| url: block.attributes.url, | ||
| }; | ||
| } else { | ||
| attributes = { | ||
| type: 'html', | ||
| content: serialize( block ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
It's totally too early for that, but a future refactor into a factory-like structure for transform/reverse transform could be a good idea to avoid a huge if/else mess.
| function gutenberg_setup_html_nav_menu_item( $menu_item ) { | ||
| if ( 'html' === $menu_item->type ) { | ||
| $menu_item->type_label = __( 'HTML', 'gutenberg' ); | ||
| $menu_item->content = ! isset( $menu_item->content ) ? get_post_meta( $menu_item->db_id, '_menu_item_content', true ) : $menu_item->content; |
There was a problem hiding this comment.
Not a blocker but just flagging this means plenty of DB roundtrips if the object cache happens to be disabled.
There was a problem hiding this comment.
It's pretty par the course for wp_setup_nav_menu_item() which is where this code will eventually live.
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/nav-menu.php#L806
There was a problem hiding this comment.
@noisysocks amazing job, I think we're almost there - this PR looks great to me! I found a single blocker which seems pretty easy to address. I am going to approve in advance, feel free to merge once that note is addressed!
Also - this could be an addition to this PR or a follow-up exploration: Should the inserter in the navigator also offer the search block?
Co-authored-by: Adam Zielinski <[email protected]>
Yes, good catch. Added to my list! 🙂 |
|
Thanks @adamziel! I'm happy with this as a foundation to build on. |
@adamziel: This already works. The problem, looking at your screenshot, is that you can't add a Search block underneath a Link block. Unsure if we should allow that. |
Right - that's a really good point! I don't think it makes sense under a Link block specifically, BUT I think it could make sense in the dropdown menu in general - just like plenty of other blocks such as images, columns, and so on. I'll find or create a new issue for that. |
|
I just created this one: #23745 |
This is a quick proof of concept for how I was thinking we could support saving non-
core/navigation-linkblocks in the Navigation Screen. I've demonstrated the approach by allowing Search to be added.Being able to add different kinds of blocks to a site's existing menu is the primary reason we're building out a block-based Navigation Screen as it will allow more WordPress users to access blocks.
I'm focused only on a technical proof of concept here. See #22096 for discussion on the UX of adding different types of blocks to Navigation.
Implementation notes
The
nav_menu_itemCPT now supports$menu_item->type = 'html'. This accompanies the existing allowed types:taxonomy,post_type,post_type_archiveandcustom. Whentypeishtmlit signifies that the menu item will render custom HTML, stored in a new$menu_item->contentfield, in place of the<a>link. The custom HTML may contain block markup.$menu_item->contentis stored in post meta. I wanted to usepost_contentbut this is already taken by$menu_item->description. I thought about usingpost_contentonly if$menu_item→type === 'html'but decided against it as this may break third party code that reads$menu_item->post_contentto access an item's description.The experimental
/menu-itemsREST API endpoint supportstype=htmland a newcontentproperty as above. The content property is split intocontent.rawandcontent.renderedso that it closely matches the/postsendpoint.content.rawmay only be accessed whencontext=edit.The approach requires minimal changes to Core that are easy for the Gutenberg plugin to shim.
wp_setup_nav_menu_itemandwp_update_nav_menu_itemmust be be updated to save and populate the new$menu_item->contentfield.Walker_Nav_Menu::start_elmust be updated to display$menu_item->contentinstead of the<a>,$args→link_before, and$args->link_after.I made no effort to make these new changes display nicely in
nav-menus.php. I don't think it's important that we do this as these changes would likely land in Core alongside anav-menus.phpreplacement.Non-
core/navigation-linkblocks do not display in Navigation blocks that are added to a post or to a FSE template. This is becausecore/navigationoverrides the link'srender_callback. Extract navigation link rendering code from the navigation block #21075 will address this.Outstanding questions
The resultant markup on the frontend looks like this:
I suspect a great many themes will not work nicely with something other than an
<a>element inside the<li>. How do we ease this transition?We definitely will want to add a filter which allows developers to disable
htmlmenu items. In addition to this, we could explore makinghtmlmenu items opt-in usingadd_theme_support().When
$menu_item->type === 'html',$menu_item->titleand$menu_item->urlwill both be set to''. This will break any third party code that blindly expects a non-empty value. I don't think this would be very common, but it's worth thinking through.I had to remove
parentfromcore/navigation-linkso that the Inserter appears when adding to a Navigation block. This means that the Navigation Link block appears in the Inserter when adding a block to a post or page, though. How do we make it so that Navigation Link can only be added to a Navigation block?Tasks
Before merge
After merge
wp_nav_menu().