Navigation: Handle block menu items #24846

noisysocks · 2020-08-27T07:00:18Z

Updates the Navigation block to properly convert menu items with type = 'block' when creating a Navigation block from an existing menu.

I also made it pull the description, rel and className over from the menu item.

How to test

Patch your current theme to support block navigation. Here's how that looks for Twenty Twenty:

Index: src/wp-content/themes/twentytwenty/functions.php
===================================================================
--- src/wp-content/themes/twentytwenty/functions.php	(revision 48655)
+++ src/wp-content/themes/twentytwenty/functions.php	(working copy)
@@ -142,6 +142,7 @@
 	$loader = new TwentyTwenty_Script_Loader();
 	add_filter( 'script_loader_tag', array( $loader, 'filter_script_loader_tag' ), 10, 2 );
 
+	add_theme_support( 'block-nav-menus' );
 }
 
 add_action( 'after_setup_theme', 'twentytwenty_theme_support' );
Index: src/wp-content/themes/twentytwenty/header.php
===================================================================
--- src/wp-content/themes/twentytwenty/header.php	(revision 48655)
+++ src/wp-content/themes/twentytwenty/header.php	(working copy)
@@ -83,25 +83,25 @@
 
 					<?php
 					if ( has_nav_menu( 'primary' ) || ! has_nav_menu( 'expanded' ) ) {
-						?>
 
-							<nav class="primary-menu-wrapper" aria-label="<?php esc_attr_e( 'Horizontal', 'twentytwenty' ); ?>" role="navigation">
+						if ( has_nav_menu( 'primary' ) ) {
 
-								<ul class="primary-menu reset-list-style">
+							wp_nav_menu(
+								array(
+									'container'      => '',
+									'items_wrap'     => '%3$s',
+									'theme_location' => 'primary',
+								)
+							);
 
-								<?php
-								if ( has_nav_menu( 'primary' ) ) {
+						} elseif ( ! has_nav_menu( 'expanded' ) ) {
+							?>
 
-									wp_nav_menu(
-										array(
-											'container'  => '',
-											'items_wrap' => '%3$s',
-											'theme_location' => 'primary',
-										)
-									);
+							<nav class="primary-menu-wrapper" aria-label="<?php esc_attr_e( 'Horizontal', 'twentytwenty' ); ?>" role="navigation">
 
-								} elseif ( ! has_nav_menu( 'expanded' ) ) {
+								<ul class="primary-menu reset-list-style">
 
+									<?php
 									wp_list_pages(
 										array(
 											'match_menu_classes' => true,
@@ -110,15 +110,14 @@
 											'walker'   => new TwentyTwenty_Walker_Page(),
 										)
 									);
+									?>
 
-								}
-								?>
-
 								</ul>
 
 							</nav><!-- .primary-menu-wrapper -->
 
-						<?php
+							<?php
+						}
 					}
 
 					if ( true === $enable_header_search || has_nav_menu( 'expanded' ) ) {

Add a Search block to a menu using the Navigation screen. See Navigation screen: Add opt-in Navigation block rendering #24503 for instructions.
Save the menu.
Go to a post and insert a Navigation block.
Select the menu and click Create.
Note that the Search block is properly added.

github-actions · 2020-08-27T07:07:39Z

Size Change: +148 B (0%)

Total Size: 1.17 MB

Filename	Size	Change
`build/block-library/index.js`	136 kB	+148 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/annotations/index.js`	3.67 kB	0 B
`build/api-fetch/index.js`	3.44 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	7.99 kB	0 B
`build/block-directory/style-rtl.css`	953 B	0 B
`build/block-directory/style.css`	952 B	0 B
`build/block-editor/index.js`	126 kB	0 B
`build/block-editor/style-rtl.css`	10.8 kB	0 B
`build/block-editor/style.css`	10.8 kB	0 B
`build/block-library/editor-rtl.css`	8.55 kB	0 B
`build/block-library/editor.css`	8.55 kB	0 B
`build/block-library/style-rtl.css`	7.47 kB	0 B
`build/block-library/style.css`	7.47 kB	0 B
`build/block-library/theme-rtl.css`	741 B	0 B
`build/block-library/theme.css`	742 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	47.7 kB	0 B
`build/components/index.js`	200 kB	0 B
`build/components/style-rtl.css`	15.5 kB	0 B
`build/components/style.css`	15.5 kB	0 B
`build/compose/index.js`	9.67 kB	0 B
`build/core-data/index.js`	12.3 kB	0 B
`build/data-controls/index.js`	1.29 kB	0 B
`build/data/index.js`	8.55 kB	0 B
`build/date/index.js`	5.38 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	568 B	0 B
`build/dom/index.js`	4.48 kB	0 B
`build/edit-navigation/index.js`	11.6 kB	0 B
`build/edit-navigation/style-rtl.css`	1.16 kB	0 B
`build/edit-navigation/style.css`	1.16 kB	0 B
`build/edit-post/index.js`	304 kB	0 B
`build/edit-post/style-rtl.css`	5.61 kB	0 B
`build/edit-post/style.css`	5.61 kB	0 B
`build/edit-site/index.js`	17 kB	0 B
`build/edit-site/style-rtl.css`	3.06 kB	0 B
`build/edit-site/style.css`	3.06 kB	0 B
`build/edit-widgets/index.js`	12 kB	0 B
`build/edit-widgets/style-rtl.css`	2.46 kB	0 B
`build/edit-widgets/style.css`	2.45 kB	0 B
`build/editor/editor-styles-rtl.css`	492 B	0 B
`build/editor/editor-styles.css`	493 B	0 B
`build/editor/index.js`	45.3 kB	0 B
`build/editor/style-rtl.css`	3.81 kB	0 B
`build/editor/style.css`	3.81 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.71 kB	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	621 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.52 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.12 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/media-utils/index.js`	5.32 kB	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/plugins/index.js`	2.56 kB	0 B
`build/primitives/index.js`	1.41 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.85 kB	0 B
`build/rich-text/index.js`	13.9 kB	0 B
`build/server-side-render/index.js`	2.77 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/url/index.js`	4.06 kB	0 B
`build/viewport/index.js`	1.85 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

noisysocks · 2020-08-27T07:43:27Z

Looks like there's some legitimate E2E test failures. I'll look into them tomorrow.

noisysocks · 2020-08-28T05:26:54Z

packages/block-library/src/navigation/placeholder.js

+		if ( menuItem.classes?.length && some( menuItem.classes ) ) {
+			attributes.className = menuItem.classes.join( ' ' );
+		}
+


Ideally we'd also set id and type here so that the URL can be dynamic. Need to think about these attributes some more, though. I've added a note to come back to this in #18345.

I'm unclear on the reasoning behind dropping these attributes, what's the background?

So the existing code in master has a few problems:

We're setting the type attribute to be menuItem.type, but this is incorrect because the attribute should be one of 'page', 'post', 'category', 'tag', or undefined and menuItem.type returns 'post_object' or 'taxonomy' IIRC.

We're setting id to menuItem.id, but this is incorrect because for a link to dynamically render its URL it will need the ID of the post, not the ID of the menu item. It should probably be set to menuItem.object_id.

Instead of fixing these issues, though, I added a note to address them when we address #18345 as there's some thinking to be done around what attributes are needed for dynamically rendering a link. IMO we should be storing three things with the block: the menu item type, the object type, the object ID. See #18345 (comment).

Thanks for the explanation, sounds like it's not something to tackle here.

It'd be good to rewrite the description on the issue with these details. 👍

noisysocks · 2020-08-28T05:47:40Z

I've updated the E2E test snapshots. This is ready for review.

Handle menu items with type = 'block' when creating a Navigation block from an existing menu.

talldan · 2020-09-25T01:57:43Z

packages/block-library/src/navigation/placeholder.js

-			innerBlocks
-		);
+			if ( ! block ) {
+				return createBlock( 'core/freeform', {


While this seems to work in testing, I think it'd be safest to also add core/freeform to the allowedBlocks of the nav block innerBlocks component.

In testing you can actually hack any block into the inner blocks of another block using the code editor (is this a bug?), but probably best not to rely on this behavior.

I do wonder if, instead, we should simply fail (loudly?) in this case. It's undefined behaviour for the menu item to have 0 or > 1 blocks in it and probably indicates a bug elsewhere. Thoughts?

Oh right, I think I've misunderstood what it's for. It actually looks like parse gives you a freeform block anyway if it's valid HTML that doesn't contain a block.

I think you're right, it should probably fail, but in a way that still makes the navigation recoverable. Not sure what the best option would be.

talldan · 2020-09-25T02:06:13Z

packages/block-library/src/navigation/placeholder.js

+		if ( menuItem.classes?.length && some( menuItem.classes ) ) {
+			attributes.className = menuItem.classes.join( ' ' );
+		}
+


I'm unclear on the reasoning behind dropping these attributes, what's the background?

talldan

Looks good in testing. The two review points can be addressed separately.

noisysocks added [Type] Bug An existing feature does not function as intended [Block] Navigation Affects the Navigation Block labels Aug 27, 2020

noisysocks requested review from ajitbohra, Soean and talldan as code owners August 27, 2020 07:00

noisysocks requested review from nerrad and ntwb as code owners August 28, 2020 05:25

noisysocks commented Aug 28, 2020

View reviewed changes

noisysocks added 2 commits September 1, 2020 11:31

Navigation: Handle block menu items

511a998

Handle menu items with type = 'block' when creating a Navigation block from an existing menu.

Navigation: Update E2E test snapshots

5c64c4b

noisysocks force-pushed the fix/handle-block-menu-items-when-creating-navigation-block branch from da4bb5f to 5c64c4b Compare September 1, 2020 01:31

talldan reviewed Sep 25, 2020

View reviewed changes

talldan approved these changes Sep 25, 2020

View reviewed changes

talldan merged commit bf0b8ab into master Sep 25, 2020

talldan deleted the fix/handle-block-menu-items-when-creating-navigation-block branch September 25, 2020 07:53

github-actions bot added this to the Gutenberg 9.1 milestone Sep 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigation: Handle block menu items #24846

Navigation: Handle block menu items #24846

noisysocks commented Aug 27, 2020 •

edited

Loading

github-actions bot commented Aug 27, 2020 •

edited

Loading

noisysocks commented Aug 27, 2020

noisysocks Aug 28, 2020 •

edited

Loading

talldan Sep 25, 2020

noisysocks Sep 25, 2020

talldan Sep 25, 2020

noisysocks commented Aug 28, 2020

talldan Sep 25, 2020

noisysocks Sep 25, 2020 •

edited

Loading

talldan Sep 25, 2020

talldan Sep 25, 2020

talldan left a comment

Navigation: Handle block menu items #24846

Navigation: Handle block menu items #24846

Conversation

noisysocks commented Aug 27, 2020 • edited Loading

How to test

github-actions bot commented Aug 27, 2020 • edited Loading

noisysocks commented Aug 27, 2020

noisysocks Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

talldan Sep 25, 2020

Choose a reason for hiding this comment

noisysocks Sep 25, 2020

Choose a reason for hiding this comment

talldan Sep 25, 2020

Choose a reason for hiding this comment

noisysocks commented Aug 28, 2020

talldan Sep 25, 2020

Choose a reason for hiding this comment

noisysocks Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

talldan Sep 25, 2020

Choose a reason for hiding this comment

talldan Sep 25, 2020

Choose a reason for hiding this comment

talldan left a comment

Choose a reason for hiding this comment

noisysocks commented Aug 27, 2020 •

edited

Loading

github-actions bot commented Aug 27, 2020 •

edited

Loading

noisysocks Aug 28, 2020 •

edited

Loading

noisysocks Sep 25, 2020 •

edited

Loading