Make the navigation block more internally consistent

[adamziel]

There's quite some code duplication in the navigation block code, leading to inconsistent behaviors when seemingly the same feature is used using a different control.

Multiple code paths for creating new menus

There are many ways a user may create a new, empty menu, and they are handled by different code paths:

gutenberg/packages/block-library/src/navigation/edit/placeholder/index.js

Lines 145 to 147 in 2f7f045

	const onCreateEmptyMenu = () => {
	onFinishMenuCreation( [] );
	};

gutenberg/packages/block-library/src/navigation/edit/placeholder/index.js

Lines 101 to 110 in 2f7f045

	const onFinishMenuCreation = async (
	blocks,
	navigationMenuTitle = null
	) => {
	const navigationMenu = await createNavigationMenu(
	navigationMenuTitle,
	blocks
	);
	onFinish( navigationMenu, blocks );
	};

gutenberg/packages/block-library/src/navigation/edit/index.js

Lines 298 to 307 in 2f7f045

	const startWithEmptyMenu = useCallback( () => {
	if ( navigationArea ) {
	setAreaMenu( 0 );
	}
	setAttributes( {
	ref: undefined,
	} );
	setIsPlaceholderShown( true );
	}, [ clientId ] );

I would like to have a single code path that is executed for each of these buttons.

Multiple data sources for dropdown menus

There are two ways of accessing a dropdown menus, and they output inconsistent items because each is handled by a different code path:

Multiple code paths for menu switching

They are more consistent with each other, but still slightly different.

Block toolbar:

gutenberg/packages/block-library/src/navigation/edit/index.js

Lines 377 to 382 in 2f7f045

	<NavigationMenuSelector
	onSelect={ ( { id } ) => {
	setRef( id );
	onClose();
	} }
	onCreateNew={ startWithEmptyMenu }

Placeholder:

gutenberg/packages/block-library/src/navigation/edit/index.js

Lines 520 to 524 in 2f7f045

	setIsPlaceholderShown( false );
	if ( post ) {
	setRef( post.id );
	}
	selectBlock( clientId );

Unintuitive logic

gutenberg/packages/block-library/src/navigation/edit/index.js

Lines 627 to 629 in 8676d2a

	( ! isEntityAvailable && ! isPlaceholderShown && (
	<PlaceholderPreview isLoading />
	) ) }

I found it confusing that we're showing a placeholder when ! isPlaceholderShown. It would be nice to have a <LoadingState /> component.

cc @noisysocks @talldan @draganescu @tellthemachines @getdave

[talldan]

The 'Navigation menu has been deleted or is unavailable' message is something I'd already deleted once (#36507).

Not sure why it was brought back again.

[adamziel]

I took that screenshot before you removed it 😅

[noisysocks]

Is this a bug or a code quality task? Wondering if we can punt it from the 5.9 board. Help me out by adding a [Type] label 😄

[adamziel]

@noisysocks I would say it's both :D but if I have to choose, it's more of a Code Quality thing. It should be fine to fix it in 6.0. I intuitively sense a larger refactor would be needed here, which means new bugs, which isn't great during the Beta period.

[Mamaduka]

I'm removing this from the 5.9 board but will add it to the Navigation one.

[getdave]

@adamziel Let's work on these items. The complexity of the block code has increased a lot in the run up to 5.9 and we need to refactor now to save ourselves pain later.

Let's break this up into tasks:

• Create loading component for Nav block.
• Consolidate code paths for Select menu dropdowns
• Consolidate code paths for code paths for creating new menus

Let's make Issues for tackling each one.

[talldan]

I took that screenshot before you removed it 😅

It's definitely back again 😓 :

gutenberg/packages/block-library/src/navigation/edit/index.js

Lines 443 to 458 in 99c72bf

	// Show a warning if the selected menu is no longer available.
	// TODO - the user should be able to select a new one?
	if ( ref && isNavigationMenuMissing ) {
	return (
	<div { ...blockProps }>
	<Warning>
	{ __(
	'Navigation menu has been deleted or is unavailable. '
	) }
	<Button onClick={ startWithEmptyMenu } variant="link">
	{ __( 'Create a new menu?' ) }
	</Button>
	</Warning>
	</div>
	);
	}

Must have accidentally been brought back as the result of a bad conflict resolution.

[talldan]

While we're talking about code quality, something else I noticed today is that useNavigationEntities does a lot. As does useNavigationMenu now. I think we could divide them into separate hooks:

• usePages
• useClassicMenus
• useClassicMenuItems
• useNavigationMenus
• useNavigationMenuPermissions

One reason is that Pages don't need to be requested all the time, only when the placeholder shows.

[getdave]

This has become a bit stale. Going to close it out.