Improve Nav Editor placeholder state

[noisysocks]

Related: #23207

Right now when you use the new Navigation screen to create a new menu, you are shown the Navigation block's placeholder which prompts you to create from an existing WordPress menu. This list contains the menu that you just created.

Could we simplify this flow for the most common case which is creating a new empty menu?

Initial placeholder state:

On selection of the nav block:

As recommended (#28124 (comment)) it could look like this:

105551625-e7fcf600-5cd0-11eb-839e-25b97d6a8f16.mp4

[talldan]

The screen and block have changed quite a bit, but this issue is still relevant. I've updated the description with some new screenshots.

The placeholder has been redesigned with full-site editing in mind. It now shows an 'empty state', which doesn't work so well in the navigation screen.

[jasmussen]

There's only ever a signle navigation block per "Menu", correct? If that's the case, can we make it appear selected always? I.e. force either is-selected, has-child-selected, or both?

That would skip the skeleton preview state.

[talldan]

Yep, we can definitely make a few customisations in the context of the screen to make the skeleton bit never show. The other issue that's harder to solve is the current menu appearing in the 'Existing' dropdown. More of a technical issue though. Let's move this one over to the Needs Dev column.

[talldan]

There's a design for this now - #28124 (comment)

It's a completely reworked placeholder, so we'll have to look into how we can replace the placeholder in the nav block only in the context of the nav screen.

[jasmussen]

Question: Is the design in #28124 (comment) actually a placeholder state? I thought it was UI that existed before any block was inserted, and not part of the Navigation Block itself. As always I will defer to you all, but it seems best if we can avoid building in navigation screen special cases into the navigation block itself.

[talldan]

Very true, that's worth exploring too.

[talldan]

So I think @jasmussen's idea above makes sense.

Essentially the nav screen could provide its own UI and we'd be deferring creation of the nav block until after 'Add all pages' or 'Start blank' is clicked.

I think there will have to be a little bit of hacking on the block to make the 'Start blank' option work (otherwise choosing it will show the block with its normal placeholder).

Add a hasPlaceholder prop for the component a bit like this hasListViewModal prop:

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

Line 44 in 57342da

hasListViewModal = true,

And use that to default isPlaceholderShown to false, and also make this placeholder prop undefined:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/navigation/edit.js#L91

[draganescu]

One other thing is that Add all pages will have to behave like it did before the pages block, since classic menus don't need this to be self updating, plus the editor won't support the block without the theme optin.

[getdave]

@critterverse Are you happy with the approach shown in the video in the description? If so I can take this on.

[javierarce]

I'll answer for @critterverse since she is working on the widget editor, and I think this mention was meant for me (but @critterverse, please jump in if you have a different idea).

I like the approach shown in the video and @jasmussen's suggestion of skipping the skeleton preview state.
Regarding the "Existing menu" dropdown… would we get rid of it, or do you want me to add it to the design?

[draganescu]

do you want me to add it to the design?

That would add a bit of more clarity which would be helpful :)

[javierarce]

Here is the modal with the "Existing menu" dropdown. I've also updated the order of the options so "start blank" appears in the first position (which seems more logical to me) and remove the down arrow.

I also think it would be nice to use the same header style in both modals:

cc: @shaunandrews

[getdave]

@javierarce Let me know when this is good to go and I'll start work on a PR.

[javierarce]

Thanks, @getdave! Let me get @shaunandrews opinion first.

Here are the designs for the mobile version. I know that we currently don't have padding around the main canvas, but I'd like to propose adding it (I'll open a new issue for that).

[shaunandrews]

I also think it would be nice to use the same header style

I'm not sure were we came up with the line, but I think it was likely from a mockup that used an accordion. I'm not sure the line is necessary.

--

Why is "Add existing menu" a primary button? Not all people will have existing menus to choose from...

[javierarce]

You are right about the line, I got it from a mockup. I've removed it.

Why is "Add existing menu" a primary button? Not all people will have existing menus to choose from...

That's true. I've updated the design so the button doesn't look so prominent and behaves like the Switch menu button (for that reason I think we don't need the arrow).

In case the user doesn't have other menus, we won't show that option (this is by the way related to this bug: the menu itself is always offered as an option in the existing menu dropdown)

[getdave]

@gwwar If we were to implement the design shown in this Issue I believe we are going to start duplicating a lot of code between

• Nav block
• Nav editor

Why? This PR is effectively implementing the Nav block placeholder state in the Nav editor. That means we'll need to duplicate all the code for:

• fetching existing pages and parsing into blocks.
• fetching menus and parsing into blocks.

Are we reaching a point where we need to place such code within a new @wordpress/navigation package and then share it between both the block and the editor? That will reduce redundancy and make maintenance easier.

I'm loath to start a major refactor but I'm interested in your opinion on alternatives before we start adding yet more code duplication.

I was thinking perhaps we could add modifiers to the block itself to allow for a simpler placeholder state but I don't believe we should start coupling the block and editor too tightly.

[jasmussen]

Regarding the nav block placeholder state, I'm afraid it remains a bit in flux. The skeleton indicators remain a source of confusion, even if small, and I've noticed an issue where the nav block when inserted in constrained contexts gets scaled improperly, so the nav block placeholder needs a bit of a refactor itself.

So it might not need to be that much of a duplication. If we're lucky, it can be a good exercise in mostly using stock components — paragraph, tertiary and primary buttons, input fields, and as little other CSS as possible.

[getdave]

Just to be clear (apologies that I wasn't!), the duplication I'm primarily concerned about is the code that queries and formats the menu/pages data into blocks. The UI is less of an issue in this case although I can imagine quite a lot of the component-level logic from placeholder.js being repeated in the Nav Editor if we go down this placeholder route.

Here are some examples of the code I'd like to reuse in the Nav Editor that currently only exists in the block:

• gutenberg/packages/block-library/src/navigation/create-data-tree.js

  Lines 16 to 37 in e901798

  	function createDataTree( dataset, id = 'id', relation = 'parent' ) {
  	const hashTable = Object.create( null );
  	const dataTree = [];
  	for ( const data of dataset ) {
  	hashTable[ data[ id ] ] = {
  	...data,
  	children: [],
  	};
  	}
  	for ( const data of dataset ) {
  	if ( data[ relation ] ) {
  	hashTable[ data[ relation ] ].children.push(
  	hashTable[ data[ id ] ]
  	);
  	} else {
  	dataTree.push( hashTable[ data[ id ] ] );
  	}
  	}
  	return dataTree;
  	}

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

  Lines 40 to 83 in e901798

  	function mapMenuItemsToBlocks( menuItems ) {
  	return menuItems.map( ( menuItem ) => {
  	if ( menuItem.type === 'block' ) {
  	const [ block ] = parse( menuItem.content.raw );
  	if ( ! block ) {
  	return createBlock( 'core/freeform', {
  	content: menuItem.content,
  	} );
  	}
  	return block;
  	}
  	const attributes = {
  	label: ! menuItem.title.rendered
  	? __( '(no title)' )
  	: menuItem.title.rendered,
  	opensInNewTab: menuItem.target === '_blank',
  	};
  	if ( menuItem.url ) {
  	attributes.url = menuItem.url;
  	}
  	if ( menuItem.description ) {
  	attributes.description = menuItem.description;
  	}
  	if ( menuItem.xfn?.length && some( menuItem.xfn ) ) {
  	attributes.rel = menuItem.xfn.join( ' ' );
  	}
  	if ( menuItem.classes?.length && some( menuItem.classes ) ) {
  	attributes.className = menuItem.classes.join( ' ' );
  	}
  	const innerBlocks = menuItem.children?.length
  	? mapMenuItemsToBlocks( menuItem.children )
  	: [];
  	return createBlock( 'core/navigation-link', attributes, innerBlocks );
  	} );
  	}

[gwwar]

Are we reaching a point where we need to place such code within a new @wordpress/navigation package and then share it between both the block and the editor? That will reduce redundancy and make maintenance easier.

@getdave That makes sense to me. I'm not sure on the package name, but likely there's going to be a need for similar code in all editors: post, site, widget, navigation. We saw this with the shared search function.

[getdave]

Looks like I need to raise an Issue/PR combo for this then.

[talldan]

'Add existing menu' is an interesting option on this screen, because it is the menu screen where these menus are created/edited.

I think it'd feel more like copying or duplicating an existing menu in this context.

@getdave That makes sense to me. I'm not sure on the package name, but likely there's going to be a need for similar code in all editors: post, site, widget, navigation. We saw this with the shared search function.

A widgets package was recently added for sharing code between the customizer and standalone widget editors. So a menus package could be similar.

I also wonder if we're reaching the point where block-library has too much in it. It has generic use everywhere blocks (paragraph/quote etc.). As well as WordPress specific blocks (latest posts). That makes me wonder if there should be wordpress/blocks and wordpress/wp-blocks packages, where the latter is not too concerned about having WordPress-y dependencies.

[jasmussen]

Interesting the you should mention "Add existing menu", as #31371 changes the phrasing to that in the block setup state.

For the block, the motivation is to make clear that it's just a starting point, a copy if you will, and that there's no connection between screen and block after that fact. Honestly, "Copy menu" might be even better phrasing to help suggest that.

[getdave]

Here's the proposal for a @wordpress/menus package.

#31580

[kellychoffman]

Locking this issue as design is ready for dev. "Locking" is for dev clarity and is not meant to dissuade further design contribution or discussion - open new issues or unlock if needed.

Comment containing final design
Figma file
Designer to add to PR for design reviews: @javierarce

[talldan]

@kellychoffman I'm not sure locking the issue is a good practice as a lot of contributors like to leave a comment volunteering to pick up an issue. That's a good idea as it prevents duplicate efforts.

I've unlocked it for now, could be worth a wider discussion on the practice.