Size Change: +877 B (0%)

Total Size: 825 kB

_{compressed-size-action}

Looking at the requests you are sending here, they seem wrong.

Requests are being sent the menus endpoint like so.
GET /__experimental/menus {locations: ["social"]}

However, this is wrong. You should be updating a menu by it's id.
This is the request you need to go.
POST /__experimental/menus/<menu_id> {locations: ["social"]}

Work noting that all locations need to be passed in the locations array. The REST API will always alright existing changes. So you want to assign main menu to all locations, the request must be

POST /__experimental/menus/<menu_id> {locations: ["social", "main"]}

Hope this is helpful.

Howdy @spacedmonkey this is weird since I see this in my tools:

So it looks like I am sending the <menu_id> . And that works. What doesn't seem to work is sending quick POST to assign menus to locations:

In this GIF you can see that if I only set one menu it is saved right, but if I try to set two menus to two locations, they get set wrongly (one of the two is not saved). Is there anything in the API that allows me one bulk call? Also why would there be a problem to send serial requests as long as they're about different menus and different locations?

These are the request I see.

Not what I am seeing. I can't see a id in those request. Also the id should not be passed as a query param in this case, should be pass in the url, like a post update.

As you can see, the above is not get request but a post.

Updating a term uses editable methods ( WP_REST_Server::EDITABLE ) which are POST, PUT, PATCH/

Another issue with the PR, is that I think that javascript will need to await for the response of the apiFetch. I think if all requests are sent at the same time, that there will be a race conditions where data is overridden.

Great catch with the "await"-ing for menu location assignment @spacedmonkey, I didn't expect that to be the need since I've always sent different id/location combos so I thought they're independent. Now with awaiting, multiple assignment in one save works all right.

@noisysocks I've addressed your review, but still need to refactor so that assigning one menu to multiple locations works.

This is almost ready, but now upon changing tabs the state is not correct as the menu locations are not fetched again and the edits are not saved in the state.

Ok, we're good for a re-review now.

Couple of tweaks to improve the code but the new changes are now sending the correct data to the api.

I really like the idea of using a hook for this! I think can we can take it a little bit further and have the hook fully encapsulate the data structure that the REST API needs and expose only an API that aligns closely with the UI of the Manage Locations tab. What do you think?

const [
	// Object that maps menu location names to an object containing menu location name, description and (possibly null) assigned menu ID.
	menuLocations, // Record<string, { name: string, description: string, assignedMenuID: number? }>
	// Function that assigns the given menu ID to the given menu location.
	assignMenuLocationMenu, // ( menuLocationName: string, menuID: number ) => void
	// Function that persists the current state of menuLocations using REST API.
	saveMenuLocations, // () => void
] = useMenuLocations();

...

<form onSubmit={ saveMenuLocations }>
	...
	{ map( menuLocations, ( menuLocation ) => (
		{ menuLocation.description }
		...
		<SelectControl
			options={ availableMenuOptions }
			value={ menuLocation.assignedMenuID }
			onChange={ ( menuID ) => {
				assignMenuLocationMenu( menuLocation.name, menuID );
			} }
		/>
	) ) }
	...
</form>

Hey @noisysocks your review suggestions are now baked in. Also, I've moved some of the form's onSubmit logic into the hook and re-renamed the variables in the MenuLocationsEditor as with the hook they are easier to grasp.

Unfortunately we need to keep MenuSelectControl and the tiny logic there b/c we have to know the previously selected value.

@draganescu sounds good!

Also, I think this should use a single save request in the grand scheme of things, not a request per menu location. That being said, it doesn't have to be in this PR - let's proceed as it is.

@draganescu it works! 👍One last note I have is that the state manipulation logic is pretty verbose. Storing state as [menuId] and {locationId: menuId} instead of {menuId: [locationIds]} could probably shave ~50 lines of code - what do you think?

Storing state as [menuId] and {locationId: menuId} instead of {menuId: [locationIds]} could probably shave ~50 lines of code - what do you think?

That is very tempting but: {menuId: [locationIds]} is still gonna be made at some point b/c that is how the API expects us to set locations, a list of them posted to a menu id. Most of the code currently in assignMenuToLocation is for storing unassigned menus. I may be missing the point here :)

Let's get this in and iterate! Thanks @draganescu!