"Manage Locations" doesn't work on the Navigation screen

[Mamaduka]

Description

Currently, it's not possible to assign menu locations to a different menu using the "Manage Locations" modal.

Step-by-step reproduction instructions

Tested using TT1 theme.

1. Go to the experimental Navigation screen.
2. Create two menus.
3. Assign both locations to the first menu.
4. Open the "Manage Locations" modal.
5. Assing both locations to the second menu.
6. Save menu and refresh page.
7. Locations aren't reassigned.

Expected Behavior

The editor shouldn't assign menu locations to the second menu.

Current Behavior

The editor doesn't assign menu locations to the second menu.

Screenshots or screen recording (optional)

CleanShot.2021-08-02.at.16.30.27.mp4

[Mamaduka]

I'm not sure if this action was ever possible. The assignMenuToLocation callback only changes data for the current menu.

gutenberg/packages/edit-navigation/src/hooks/use-menu-locations.js

Lines 44 to 62 in f53785f

	const assignMenuToLocation = useCallback(
	async ( locationName, newMenuId ) => {
	const oldMenuId = menuLocationsByName[ locationName ].menu;
	const newMenuLocationsByName = merge( menuLocationsByName, {
	[ locationName ]: { menu: newMenuId },
	} );
	setMenuLocationsByName( newMenuLocationsByName );
	const activeMenuId = newMenuId || oldMenuId;
	editMenuEntityRecord( ...menuEntityData, {
	locations: locationsForMenuId(
	newMenuLocationsByName,
	activeMenuId
	),
	} );
	},
	[ menuLocationsByName ]

I think these assignments should be handled by __experimental/menu-locations, which currently is read-only.

/cc @getdave, @talldan, @spacedmonkey.

[draganescu]

I tested this and indeed the modal does not set the location, only the UI is updated.

[Mamaduka]

Thanks for testing, @draganescu.

Do you think we should update the menu-locations endpoint to handle updating multiple locations?

[talldan]

@Mamaduka It may also be possible to use the batch API for this kind of thing.

Would be good to get input from the REST API team. I see Jonny (spacedmonkey) has already been pinged. @TimothyBJacobs do you have any thoughts?

[TimothyBJacobs]

A menu's location is managed via the menus endpoint. Is there a reason we can't use that endpoint here?

gutenberg/lib/class-wp-rest-menus-controller.php

Line 566 in 9b5a401

protected function handle_locations( $menu_id, $request ) {

[spacedmonkey]

The menus endpoint should be used here. In my testing a request like this worked.

await wp.apiFetch({path: "__experimental/menus/43", data: { locations: ['primary'] }, method: 'POST'})

Menu locations are a static endpoint, like taxonomies or post types. Menu locations assignment happens at the menu level.

[talldan]

I think the issue is that the UI allows for assigning multiple menus to locations at once.

So I guess in this case it'd be best to batch multiple requests to the menus endpoint @TimothyBJacobs @spacedmonkey?

The modal should probably also have its own save button:

Because here we're managing multiple menus, but the save button at the top of the editor only saves the current menu (which is the root problem here).

[anton-vlasenko]

Because here we're managing multiple menus, but the save button at the top of the editor only saves the current menu (which is the root problem here).

Yes, I agree. That's the root problem.
What if we just fix how it saves menu locations so that we don't have to add another "Save" button to the modal?
Theoretically, if we fix this we won't have to change anything else.

[talldan]

I feel like a dedicated Save button could be a better experience, a bit like the current menu screen. But I'd defer to @javierarce's opinion.

I noticed this morning there was some design feedback on the tracking issue mentioning the location of Manage Locations - #29102 (comment), so maybe this could be revisited anyway.

edit: There's an issue to tidy up some of the rough edges of the editor's main save button - #31813.

[TimothyBJacobs]

So I guess in this case it'd be best to batch multiple requests to the menus endpoint @TimothyBJacobs @spacedmonkey?

Yep!

[javierarce]

I feel like a dedicated Save button could be a better experience, a bit like the current menu screen. But I'd defer to @javierarce's opinion.

The sidebar (and its save button) should only affect the current menu, so I agree with @talldan: we need to have a save button inside the modal and we should also show a snackbar message to indicate that the locations were updated.

Otherwise, it gets super confusing if, for example, I reassign locations and then click the edit button next to an item. In that case, I'm not sure if I'll need to click save in the new menu or what happened to the other locations. Did I lose those changes? It's difficult to tell.

In any case, I think we'll need to revisit the interface of this modal and the corresponding section of the sidebar to make it less confusing.