Navigation Editor: Allow menu renaming #29012
Merged
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
grzim
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
6 tasks
|
Size Change: +521 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
talldan
reviewed
Feb 16, 2021
grzim
added
[Feature] Navigation Screen
[Type] Feature
…tton, when clicked name editor is focused
grzim
commented
Feb 17, 2021
This was referenced Feb 18, 2021
Closed
talldan
reviewed
Feb 18, 2021
added 2 commits
February 18, 2021 14:56
…o preserve the same order of items in navbar
talldan
reviewed
Feb 19, 2021
packages/edit-navigation/src/hooks/use-navigation-editor-menu.js
Outdated
Show resolved
Hide resolved
Comment on lines
25
to
26
| saveNavigationPost( navigationPost ); | ||
| saveMenuName(); |
There was a problem hiding this comment.
I didn't anticipate this, but both these lines result in the menu being saved. saveNavigationPost saves the menu via the customizer API (because the REST API doesn't quite work yet for saving menu items).
I'd be worried about race conditions here.
This is some technical debt, so not really something caused by your PR.
I don't have a solution available right now, will have to have a think about it.
There was a problem hiding this comment.
what is the edge case when the race condition could really happen?
There was a problem hiding this comment.
I'm worried that one request might overwrite the other, since both API requests can modify the same database record.
I think the easiest solution right now would be to make sure the requests happen sequentially. This could be done by moving the saveEditedEntityRecord dispatch to be inside the saveNavigationPost action. Should be able to dispatch it with code like this, where the yield will wait for the request to complete before triggering the second request (the call to batchSave):
yield dispatch(
'core',
'saveEditedEntityRecord',
'root',
'menu',
menuId
);This makes sense actually, as it means the on-screen notices that are implemented in saveNavigationPost can be made so that the visible notices associated with saving are displayed at the right time.
There was a problem hiding this comment.
Some further background. The way saving menus currently works is a temporary thing. Currently the batchSave function has been patched to use an endpoint that is currently used by the Customize > Menus screen. But that isn't part of the REST API, and not something we want to support long into the future for the navigation editor.
Ideally the whole system would use the WordPress REST APIs and the core-data package's entities. There are two things that need to happen:
- Add a way to reorder menu items using the REST API (REST API: Introduce reorder menu endpoint #25093)
- Update the navigation editor to use the core-data package's newly introduced batch save API (from Core Data: Add __experimentalBatch() #28210) for saving menus and menu items.
|
@shaunandrews Can those be addressed separately? The help text is from the TextControl components styling, so is something that should be addressed in a separate change. I'd be pretty keen to get this PR shipped as soon as @grzim wraps up the code review changes, as this is an important missing feature for the screen. I realise the submit button is also missing because of my advice in reviewing the previous PR before your designs were posted. Maybe I can make up for that by addressing that in a follow-up PR myself. |
|
Fine by me. Lets keep improving. |
7 tasks
talldan
reviewed
Mar 3, 2021
7 tasks
|
I tried rebasing this. Fixing the conflict is easy ( |
…s/gutenberg into add/navigation-menu-name-editor
talldan
approved these changes
Mar 8, 2021
talldan
changed the title
talldan
force-pushed
the
add/navigation-menu-name-editor
branch
from
9b5bf3b to
2f53f2f
Compare
This was referenced Mar 10, 2021
7 tasks
|
Should we do the same treatment for Reusable blocks? |
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
Advances #28856
Advances #28864
This PR includes a menu name editor placed in the navbar.
The menu name is displayed in the toolbar - upon clicking on it, the menu editor is focused.
According to design #28864 (comment)
How has this been tested?
Tested manually
Screenshots
Types of changes
New feature
Checklist: