Navigation Editor: Fix saving locations using the "Manage Locations" popup #34714
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anton-vlasenko
requested review from
adamziel,
draganescu,
getdave,
kevin940726,
spacedmonkey,
talldan,
tellthemachines and
TimothyBJacobs
as code owners
|
Size Change: +253 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
anton-vlasenko
force-pushed
the
fix/manage-locations-doesnt-work-on-the-navigations-screen
branch
from
919cf75 to
46c8182
Compare
anton-vlasenko
changed the title
anton-vlasenko
changed the title
anton-vlasenko
changed the title
anton-vlasenko
changed the title
| * @see WP_REST_Controller | ||
| */ | ||
| class WP_REST_Menus_Controller_Test extends WP_REST_Menus_Controller { | ||
| public function test_it_allows_batch_requests_when_updating_menus() { |
There was a problem hiding this comment.
Move this method to class-rest-nav-menus-controller-test.php and delete this class.
There was a problem hiding this comment.
Thank you for the review.
I thought the test class must be named according to the class it is testing.
/__experimental/menus/(?P<id>[\d]+) endpoint is defined inside the WP_REST_Menus_Controller class.
Shouldn't we name the test class WP_REST_Menus_Controller_Test then?
There was a problem hiding this comment.
@anton-vlasenko ideally that would be the case. In reality, we don't follow this structure in tests at the moment. All the tests for that /__experimental/menus endpoint are in the file @spacedmonkey mentioned so it makes sense to keep adding new tests there.
As for why we have such a naming structure, I am not sure. Perhaps it's to avoid name conflicts when these classes are later migrated to WP core? @gziolo might have more context here.
There was a problem hiding this comment.
When me and Tim migrate this to core, I will make sure to name the test class correctly
There was a problem hiding this comment.
Thank you for the explanation, @adamziel .
I was just curious why we don't follow that structure.
Fixed in 67ea4e4e06d8efbb6df6036800c336d4856ae249
|
Some small tweaks to PHP, but the JS looks and works well! |
|
@kevin940726 @getdave @adamziel |
adamziel
reviewed
Sep 10, 2021
Comment on lines
42
to
86
| const batch = createBatch(); | ||
| menus.forEach( ( { id } ) => { | ||
| const locations = menuLocations | ||
| .filter( ( menuLocation ) => menuLocation.menu === id ) | ||
| .map( ( menuLocation ) => menuLocation.name ); | ||
|
|
||
| batch.add( { | ||
| path: `/__experimental/menus/${ id }`, | ||
| data: { | ||
| locations, | ||
| }, | ||
| method: 'POST', | ||
| } ); | ||
| } ); | ||
|
|
||
| const isSuccess = await batch.run(); | ||
| if ( isSuccess ) { | ||
| createSuccessNotice( __( 'Menu locations have been updated.' ), { | ||
| type: 'snackbar', | ||
| } ); | ||
| closeModal(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
@anton-vlasenko The bad news is that createBatch is an experimental API and we don't want to export it from core-data – this would make it a public API that developers may start relying on.
The good news is that you don't need to do that. You could add something along these lines to actions.js:
export const updateMenuLocations = ( menus, menuLocations ) => async ({ registry }) => {
const tasks = menus.map( ( { id } ) => {
const locations = menuLocations
.filter( ( menuLocation ) => menuLocation.menu === id )
.map( ( menuLocation ) => menuLocation.name );
return ({ saveEntityRecord }) => saveEntityRecord( 'root', 'menu', {
locations
} );
} );
return await registry
.dispatch( 'core' )
.__experimentalBatch( tasks );
}And then do something like this to your component:
const { updateMenuLocations } = useDispatch( navigationStore );
const handleUpdate = async () => {
try {
await updateMenuLocations( menus, menuLocations );
createSuccessNotice( __( 'Menu locations have been updated.' ), {
type: 'snackbar',
} );
closeModal();
} catch( e ) {
createErrorNotice( __( 'There was an error.' ), {
type: 'snackbar',
} );
}
}You will also need to add __experimentalUseThunks: true to store definition.
There are a few caveats here. For once, handling errors is pretty tricky. You will need to check for getLastEntitySaveError for each saved menu:
registry
.select( 'core' )
.getLastEntitySaveError( kind, entityType, change.id )Ideally the batch would just throw an exception when things fail, but unfortunately this isn't a case now. We may rework batching soon, but it is what it is for now.
There was a problem hiding this comment.
We don't have much documentation on __experimentalBatch, but we use it in the edit-widgets package: https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-widgets/src/store/actions.js#L95
(It's all based in generators so you'll see a lot of yield statements, that's an old way. New code can all rely on async/await)
There was a problem hiding this comment.
@adamziel how come it does not have an experimental name?
There was a problem hiding this comment.
@draganescu I think the purpose of __experimental is to warn others. It makes sense for exported things, but createBatch() is not exported anywhere. @noisysocks who implemented it may have more context
There was a problem hiding this comment.
Thank you for such a great comment, @adamziel ! I owe you. I've learned a lot from it.
I've refactored my code and now it doesn't depend on the createBatch function.
I have a question.
- Is your suggestion to add a new method to the
actions.jssomething that is nice to have? Or we must do it? I'm asking because I'm not sure how much time I'm allowed to spend on this task. I'm not so familiar with the core-data package. - We've agreed that we will use
experimental/menus/${menuId}endpoint to send requests to update menus' locations. Do you know if your suggestion uses that endpoint to update menus under the hood?
There was a problem hiding this comment.
- we can't export createBatch() from core-data so this one is a blocker
- I haven't actually tested that but I think so, refer to entities.js in core-data whenever in doubt. I'll only be able to confirm on Tuesday so might be faster for you to check than wait for me
There was a problem hiding this comment.
@adamziel I would leave it as is because I've refactored my PR and it doesn't depend on createBatch() now.
If you insist that we need to add a new method to the actions.js file and use core-data package, so be it. Please let me know and I'll go that route.
I'd like to leave this PR as is mainly because of the amount of time I've already spent on it. Time is my main concern here.
Thank you again for all the input.
anton-vlasenko
changed the title
It's needed to save menu locations.
It's better to put all "save" related logic into the manage-locations component.
2. Small refactoring.
…need to use batch API to save menus' locations in the Manage Locations popup.
…ches to the batch REST API endpoint. The previous method doesn't properly parse error responses.
Co-authored-by: Jonny Harris <[email protected]>
…ethods, so I'm just moving it above them. 2. Update test fail message.
…rivate methods, so I'm just moving it above them." This reverts commit 400236d305ca4cb18da40f568c1bb09bd0231a38.
This reverts commit 72440411ad261191987c856de041a7be19962783.
…lly." This reverts commit b7fc4a12a2bd4353c131b994eff8fdc1d202cf5b.
We can't use createBatch function from wordpress/core-data.
anton-vlasenko
force-pushed
the
fix/manage-locations-doesnt-work-on-the-navigations-screen
branch
from
bc6555e to
8f5b433
Compare
anton-vlasenko
added
[Feature] Navigation Screen
[Feature] Saving
|
Thanks @anton-vlasenko! |
talldan
changed the title
talldan
deleted the
fix/manage-locations-doesnt-work-on-the-navigations-screen
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR aims to give users the ability to assign all available menus to an arbitrary location inside the
Manage Locationspopup.Currently it's only possible to assign a location to a menu that is being edited.
Fixes #33815.
This PR doesn't fix the issue with the main "Save" button because we have another issue for that.
How has this been tested?
This has been tested using the
Twenty Twenty-OneWordPress theme.Go to the experimental Navigation screen.
Create two menus.
Switch to the first menu using the
Switch Menulink at the top of the page.Open the
Manage Locationspopup.Assign the first location to the first menu.
Assign the second location to the second menu.
Click the
Updatebutton.You should see
Menu locations have been updatedmessage at the bottom of the page.Refresh the page.
Make sure that the first menu is assigned to the first location.
Switch to the second menu using the
Switch Menulink at the top of the page.Make sure that the second menu is assigned to the second location.
Types of changes
Bug fix ( but this also can be considered as a new feature).
Checklist:
*.native.jsfiles for terms that need renaming or removal).