Navigation screen: Atomic save using customizer API endpoint #22603
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
adamziel
added
[Feature] List View
|
Size Change: -18.3 kB (1%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
adamziel
changed the title
adamziel
requested review from
tellthemachines,
noisysocks,
draganescu and
talldan
adamziel
commented
May 26, 2020
Comment on lines
+111
to
+121
| createErrorNotice( __( 'There was an error.' ), { | ||
| type: 'snackbar', | ||
| } ); |
There was a problem hiding this comment.
We should implement a more sophisticated error handling in a follow-up PR
adamziel
commented
May 26, 2020
|
|
||
| export default function useNavigationBlocks( menuId ) { | ||
| const [ deletedMenuItemsIds, setDeletedMenuItemsIds ] = useState( [] ); |
There was a problem hiding this comment.
This is a hack to work around re-appearing menu items. I'd like to get rid of it and only rely on the data layer in a follow-up PR.
There was a problem hiding this comment.
Merging the delete part will make it work.
adamziel
commented
May 26, 2020
Comment on lines
69
to
77
| promiseQueueRef.current.enqueue( () => | ||
| createDraftMenuItem( clientId ).then( | ||
| async ( menuItem ) => { | ||
| menuItemsRef.current[ clientId ] = menuItem; | ||
| processedBlocksIds.current.splice( | ||
| processedBlocksIds.current.indexOf( clientId ) | ||
| ); | ||
| } | ||
| ) |
There was a problem hiding this comment.
We should handle failures here - let's discuss in a follow-up PR.
adamziel
commented
May 26, 2020
| createSuccessNotice( __( 'Navigation saved.' ), { | ||
| type: 'snackbar', | ||
| } ); | ||
| receiveEntityRecords( 'root', 'menuItem', [], query, true ); |
There was a problem hiding this comment.
This re-requests menu items related to current menu. Ideally we won't need to do it - let's discuss in a follow-up PR.
draganescu
reviewed
May 26, 2020
|
|
||
| export default function useNavigationBlocks( menuId ) { | ||
| const [ deletedMenuItemsIds, setDeletedMenuItemsIds ] = useState( [] ); |
There was a problem hiding this comment.
Merging the delete part will make it work.
draganescu
reviewed
May 26, 2020
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Show resolved
Hide resolved
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
| useEffect( | ||
| function() { | ||
| for ( const clientId of getAllClientIds( debouncedBlocks ) ) { | ||
| // Already has a related menu item |
There was a problem hiding this comment.
What does "related" mean here?
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
| ); | ||
| setDeletedMenuItemsIds( [ | ||
| ...deletedMenuItemsIds, | ||
| difference( allMenuItemsIds, savedMenuItemIds ), |
There was a problem hiding this comment.
In 21557 I fixed an issue where we delete all the nested items because of a bad difference.
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Show resolved
Hide resolved
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
|
With this kind of manual |
@draganescu while out of scope for this PR, this is a pretty good question! Would you open an issue to discuss that more? |
|
@adamziel here is how it works for me, can you reproduce this behavior (Firefox)
I used to have this problem when I attempted to save a state version of menu items in my deleteEntity explorations. |
This was referenced May 28, 2020
|
@draganescu ah thank you for reporting. I just took another pass on it and turns out |
6 tasks
adamziel
force-pushed
the
add/navigation-atomic-save-customizer-endpoint
branch
from
fa7be78 to
1ff5329
Compare
6 tasks
…g it duplicates {n} results where n is the amount of the deleted items. The underlying state is intact and we should address the root cause in a separate PR.
adamziel
force-pushed
the
add/navigation-atomic-save-customizer-endpoint
branch
from
1ff5329 to
b6315f4
Compare
|
The data layer PR is now merged and I removed the workarounds in this one - it looks good in my testing. |
draganescu
approved these changes
May 29, 2020
oandregal
changed the title
12 tasks
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 enables saving the entire navigation using a single HTTP request.
The current behavior is sending one request per change at the moment. If you e.g. add 20 menu items, update 17 and remove 10, it will result in 47 http requests. This is pretty fragile and would easily break under a number of scenarios - validation errors, network failures, accidentally closing the browser, and so on.
This PR proposes the following implementation:
The previous approach also proposed a new batch endpoint for handling these requests - ultimately we agreed on implementing this on the core level instead of adding a custom endpoint: #22148
How has this been tested?
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: