Navigation screen: Add opt-in Navigation block rendering #24503
Conversation
|
@noisysocks I'd like to see a way here for wp_nav_menu to be used in conjunction with blocks, and for plugins to be able to switch it on block support without affecting other locations in the theme. I don't know the exact answer, but I think there needs to be further thought on how adding theme_support('block-nav-menus') applies to each instance of wp_nav_menu. Turning it on would mean the theme author would need to update every menu on their theme, but they won't be able to account for custom menus that their users have added, and it would wipe out any user modifications made using filters/actions/plugins that only walkers support (on that note, there would need to be some mechanism for the user to opt out of the block html navigation and go back to the walker). And the other way around, if a plugin added theme_support('block-nav-menus'), it would apply to all menu locations in the theme. In short I think a more granular approach is needed:
(Terrible naming but you get the idea!) Allowing 3 situations to be accounted for:
(all 3 may be used in conjunction on the same site/theme) |
|
Hey @manmotive, thanks for the feedback. Making this setting more granular is a good idea. How about we make it a per menu location setting instead of a per theme setting by updating register_nav_menu(
'primary',
array(
'description' => __( 'Desktop Horizontal Menu' ),
'supports' => array(
'blocks' => true,
),
)
);It's a straightforward change but it will require Core changes because |
noisysocks
force-pushed
the
try/have-themes-opt-in-to-blocks-in-navigation
branch
from
0715328
to
825bcae
Compare
noisysocks
changed the title
noisysocks
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
Allows themes to opt-in to having `wp_nav_menu()` render a complete Navigation block in place of the menu. This allows arbitrarily complex Navigation block structures can be used in an existing theme. When a theme declares `add_theme_support( 'block-nav-menus' )`, users are able to add non-link blocks to a Navigation using the Navigation screen. This block tree is stored as menu items and re-assembled into a Navigation block for display using `render_block()` in the frontend. Non-link blocks are stored using a menu item with type `'block'` which renders properly in `nav-menus.php`. This allows users to switch to a theme that does not support block menus and still see their data in WP Admin.
noisysocks
force-pushed
the
try/have-themes-opt-in-to-blocks-in-navigation
branch
from
825bcae
to
3a47a1a
Compare
|
Size Change: -32 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
This is ready for review. I updated the PR description with a demo GIF and testing steps. This now supports storing arbitrarily complex block trees which you can test for yourself by adding e.g. Here's a very contrived example of how a
|
lib/navigation.php
Outdated
| */ | ||
| function gutenberg_setup_block_nav_menu_item( $menu_item ) { | ||
| if ( 'block' === $menu_item->type ) { | ||
| $menu_item->type_label = __( 'Gutenberg Block', 'gutenberg' ); |
There was a problem hiding this comment.
Could probably just go for 'Block' here (and for the title).
At least, I don't think the word 'Gutenberg' should be in the UI.
It'd be awesome to have the block name, but that'd probably be hard work.
There was a problem hiding this comment.
I second the point to remove the word "Gutenberg" :) Just "Block", or yes, ideally, the block's name.
There was a problem hiding this comment.
It'd be awesome to have the block name, but that'd probably be hard work.
We could display the block's name (e.g. "core/image") but I'm not sure that that's very useful to users.
I changed it to 'Block' in 50d192f. I updated the widgets screen, too, for consistency.
| * | ||
| * @see https://core.trac.wordpress.org/ticket/50544 | ||
| * | ||
| * @param array $menu_items The menu items, sorted by each menu item's menu order. |
There was a problem hiding this comment.
I notice some of the functions are missing @return documentation.
| ), | ||
| ); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
For safety, I feel like it'd be good to check the type here as well.
There was a problem hiding this comment.
I think it's OK. The type is either going to be one of the Core types (custom, object, etc.) or a totally custom one that a plugin adds. Either way we should use title and url to create a link. At least until #22919, that is.
| $block = array( | ||
| 'blockName' => 'core/navigation-link', | ||
| 'attrs' => array( | ||
| 'label' => $menu_item->title, | ||
| 'url' => $menu_item->url, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Seems like a good reason to use variations if this issue is implemented:
#22919
Otherwise each block will require a way to handle the menu item types.
lib/navigation.php
Outdated
| if ( 'block' === $item->type ) { | ||
| ?> | ||
| <p class="description description-wide"> | ||
| <textarea readonly><?php echo esc_textarea( trim( $item->content ) ); ?></textarea> |
There was a problem hiding this comment.
Looks like this needs a label.
lib/navigation.php
Outdated
| function gutenberg_add_block_menu_item_styles_to_nav_menus( $hook ) { | ||
| if ( 'nav-menus.php' === $hook ) { | ||
| $css = <<<CSS | ||
| /** | ||
| * HACK: We're hiding the description field using CSS because this | ||
| * cannot be done using a filter. When merged to Core, we should | ||
| * actually remove the field from | ||
| * `Walker_Nav_Menu_Edit::start_el()`. | ||
| */ | ||
| .menu-item-block .description:first-child { | ||
| display: none; | ||
| } | ||
|
|
||
| .menu-item-block textarea { | ||
| width: 100%; | ||
| min-height: 100px; | ||
| } | ||
| CSS; | ||
| wp_add_inline_style( 'nav-menus', $css ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This doesn't take into account that the user might have other fields enabled in screen options.
Might be best to add a unique class to the custom field wrapper, and then use :not to hide all the others.
Having said that, I wonder if we can get away with just CSS hacks. An issue I spotted is that it's possible to nest blocks in the nav-menus screen in a way that's incorrect (e.g. adding a link to search) ...
There was a problem hiding this comment.
it's possible to nest blocks in the nav-menus screen in a way that's incorrect (e.g. adding a link to search) .
That is something that feels funny for some reason. If they're menu items I am not sure we can make them un-nestable.
There was a problem hiding this comment.
Might be best to add a unique class to the custom field wrapper, and then use :not to hide all the others.
Done in 641aebb. It relies on extra fields having the .description class which isn't necessarily the case for e.g. third party plugins. I think it's the best we can do without patching Core.
Having said that, I wonder if we can get away with just CSS hacks. An issue I spotted is that it's possible to nest blocks in the nav-menus screen in a way that's incorrect (e.g. adding a link to search) ...
That's an interesting point... maybe we could disable re-ordering Block menu items? I'm not sure.
There was a problem hiding this comment.
I wonder if we should not allow using this UI at all if the menu contains blocks.
Is there a purpose to have multiple interfaces for the same thing?
There was a problem hiding this comment.
We'll need to support disabling the new screen and functionality entirely (e.g. using the Classic Editor plugin) because there will be some users that prefer the old screen and some setups that rely on the nav-menus.php DOM structure.
If the new screen and functionality is disabled, I don't want any blocks that were inserted into a menu to go "missing". Hence the need to display them (in a very limited way) in nav-menus.php.
| function disableInsertingNonNavigationBlocks( settings, name ) { | ||
| if ( ! [ 'core/navigation', 'core/navigation-link' ].includes( name ) ) { | ||
| set( settings, [ 'supports', 'inserter' ], false ); | ||
| } | ||
| return settings; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could also just register the blocks we want instead of registering every core and experimental block and then disabling inserter support.
There was a problem hiding this comment.
The idea is to have them registered but not insertable so that, when a user switches away from a theme that supports blocks, they'll still see the blocks that they previously added to their menu. This way we're not "losing" any of the user's data.
We'll need to add some sort of UI which indicates that the invalid blocks won't display in the frontend. I think best to do that separately since it requires design.
There was a problem hiding this comment.
I see what you mean, though not registering the blocks seems fairly close to what you describe. If they're not registered, we wouldn't lose the data, but the block would be shown as unrecognized/missing.
For this page we could also consider registering a different 'missing' block that handles the messaging better:
https://github.com/WordPress/gutenberg/tree/master/packages/block-library/src/missing
Perhaps move this particular part out into a separate issue.
There was a problem hiding this comment.
For this page we could also consider registering a different 'missing' block that handles the messaging better:
https://github.com/WordPress/gutenberg/tree/master/packages/block-library/src/missing
Good idea!
Perhaps move this particular part out into a separate issue.
Agreed.
There was a problem hiding this comment.
I have demoed this twice by now and it works quite (amazingly) well! Aside from @talldan 's review bits, it is a solid next step and quite merge-able.
There was a problem hiding this comment.
Looks good to me, nice work here! Just fix that issue with the name attribute before merging. 🚀
I think there are a couple of follow-ups:
- Decide how to deal with non-navigation blocks on the old nav screen (e.g. disallow moving them to prevent invalid nesting?)
- Decide how to deal with non-navigation blocks on the new nav screen for themes that opt out of the feature.
| @@ -323,7 +336,7 @@ function gutenberg_output_block_menu_item_custom_fields( $item_id, $item ) { | |||
| <p class="field-content description description-wide"> | |||
| <label for="edit-menu-item-content-<?php echo $item_id; ?>"> | |||
| <?php _e( 'Content', 'gutenberg' ); ?><br /> | |||
| <textarea readonly><?php echo esc_textarea( trim( $item->content ) ); ?></textarea> | |||
| <textarea readonly name="menu-item-content[<?php echo $item_id; ?>]"><?php echo esc_textarea( trim( $item->content ) ); ?></textarea> | |||
There was a problem hiding this comment.
Seeing some square brackets in the in the interpolation for the name here:
There was a problem hiding this comment.
That's correct. PHP interprets form values with [] as an array.
https://www.php.net/manual/en/reserved.variables.post.php#87650
Implements the majority of #24364. Continues on from the work started in #22656.
Allows themes to opt-in to having
wp_nav_menu()render a complete Navigation block in place of the menu. This allows arbitrarily complex Navigation block structures can be used in an existing theme.When a theme declares
add_theme_support( 'block-nav-menus' ), users are able to add non-link blocks to a Navigation using the Navigation screen.This block tree is stored as menu items and re-assembled into a Navigation block for display using
render_block()in the frontend.If the user switches to a theme that does not support block menus, or disables this functionality, non-link blocks are no longer rendered in the frontend. Care is taken, however, to ensure that users can still see their data in
nav-menus.php.See #24364 for more information on the approach.
How to test
Note that in Gutenberg > Navigation (beta), you can only add Link blocks to your Navigation. Note also that the frontend renders regular nav menu markup.
Patch your current theme to support block navigation. Here's how that looks for Twenty Twenty:
The frontend should now render Navigation block markup.
Go back to Gutenberg > Navigation (beta) and add a Search block. The Search block should now display on the frontend.
Switch to a different theme. The frontend should now display regular nav menu markup without a Search block.
Go back to Gutenberg > Navigation (beta). The editor should now not allow a Search block to be inserted. The existing Search block should still be displayed.
Go to Appearance > Menus. A Gutenberg Block widget should appear containing the Search block.
Follow up tasks