Allow renaming, duplication and deleting of Navigation menus from Browse Mode Sidebar #50880
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
MaggieCabrera
commented
May 23, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
|
Size Change: +806 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
MaggieCabrera
force-pushed
the
browse-mode-navigation-options
branch
from
5c18e6a
to
5296e28
Compare
|
Flaky tests detected in e49e2ec. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5241888040
|
MaggieCabrera
force-pushed
the
browse-mode-navigation-options
branch
from
bc5fc4f
to
99f65f2
Compare
|
I have a few questions on behavior:
|
draganescu
reviewed
Jun 2, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Jun 2, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
MaggieCabrera
force-pushed
the
browse-mode-navigation-options
branch
from
a7feb63
to
b71de84
Compare
MaggieCabrera
requested review from
scruffian,
getdave,
jeryj,
jasmussen and
ajlende
|
Nice work. This one is close to land. Here's a GIF duplicating a menu, renaming one, and deleting one. It's coming together well:
A few small things. The popover menu should not be the 
It should use the default gray bordered and shadowed popover style like this: 
The focus style on the input inside the modal is cropped off: This could be a separate issue that should be fixed in the modal component itself. In case it is, it should not be fixed locally, only globally. It's really nice to be able to delete a menu here, nice. But I wonder if it shouldn't have an "Are you sure" confirm? 
After these, this one should be good to go. Thanks! |
getdave
reviewed
Jun 6, 2023
There was a problem hiding this comment.
Thank you for this PR. It is looking really good.
Let's do some code quality work upfront to make the files easier to reason about and maintain going forward.
We also need to address the error handling which includes:
- displaying appropriate notices to the user
- taking the appropriate action in the UI (if required)
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Jun 6, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Jun 6, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Jun 6, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Jun 6, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
getdave
force-pushed
the
browse-mode-navigation-options
branch
from
b71de84
to
223c67b
Compare
getdave
previously requested changes
Jun 9, 2023
There was a problem hiding this comment.
Update: this is now fixed.
Screen.Capture.on.2023-06-09.at.10-28-54.mp4
I found a bug here. Because we're using editEntityRecord it's making changes to the record in state even when we dismiss the modal. That's not what we want. We can keep the rename state local to the modal.
This comment was marked as resolved.
This comment was marked as resolved.
getdave
reviewed
Jun 9, 2023
| @@ -74,6 +116,14 @@ export default function SidebarNavigationScreenNavigationMenu() { | |||
|
|
|||
| return ( | |||
| <SidebarNavigationScreenWrapper | |||
| actions={ | |||
| <ScreenNavigationMoreMenu | |||
| menuTitle={ decodeEntities( menuTitle ) } | |||
There was a problem hiding this comment.
Suggested change
| menuTitle={ decodeEntities( menuTitle ) } | |
| menuTitle={ menuTitle } |
Let's pass the raw value here and allow the ScreenNavigationMoreMenu to handle output.
getdave
reviewed
Jun 9, 2023
| import { useState } from '@wordpress/element'; | ||
|
|
||
| export default function RenameModal( { menuTitle, onClose, onSave } ) { | ||
| const [ editedMenuTitle, setEditedMenuTitle ] = useState( menuTitle ); |
There was a problem hiding this comment.
We'll need to decodeEntities here and also ensure the button disabled check accounts for this.
getdave
reviewed
Jun 9, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/rename-modal.js
Outdated
Show resolved
Hide resolved
|
@jasmussen I have fixed the following from your review: Input focus clipping
More menu popover styling
I also added a confirmatory step to the Delete action |
getdave
requested review from
draganescu,
getdave and
scruffian
and removed request for
getdave
getdave
reviewed
Jun 9, 2023
| <VStack spacing="3"> | ||
| <p> | ||
| { __( | ||
| 'Are you sure you wish to delete this Navigation menu?' |
There was a problem hiding this comment.
@jasmussen Are you happy with this wording or would you prefer want over wish?
There was a problem hiding this comment.
Small preference for want, but not strong at all.
getdave
reviewed
Jun 9, 2023
Comment on lines
+85
to
+98
| const originalRecord = getEditedEntityRecord( | ||
| 'postType', | ||
| 'wp_navigation', | ||
| postId | ||
| ); | ||
|
|
||
| // Apply the edits. | ||
| editEntityRecord( 'postType', postType, postId, edits ); | ||
|
|
||
| // Attempt to persist. | ||
| try { | ||
| await saveEditedEntityRecord( 'postType', postType, postId, { | ||
| throwOnError: true, | ||
| } ); |
There was a problem hiding this comment.
This whole edit then save flow seems really clunky. I'm not sure if there is a better way but if there is I'm keen to learn. Currently we're having to save a copy of the record prior to editing in order that we can revert afterwards.
Note that using saveEntityRecord directly doesn't work as expected. You have to use editEntityRecord in conjunction with saveEditedEntityRecord (note the "Edited") to make it work.
There was a problem hiding this comment.
yeah, this was confusing to me too!
getdave
changed the title
scruffian
reviewed
Jun 9, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/index.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Jun 9, 2023
|
For the clipped input, I'm fine with moving forward to not bog down things in details, but a few quick notes just to capture them:
Thanks for the followup Dave! |
|
Thank you so much for picking this one up. It's looking really good. Thank you for commenting the code so effectively as always, it goes a long way. I'm going to defer to someone else for the ✅ but this is looking great so far. |
getdave
reviewed
Jun 12, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menu/delete-modal.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Jun 12, 2023
| } | ||
| } | ||
|
|
||
| .sidebar-navigation__rename-modal-form { |
There was a problem hiding this comment.
Should we remove this and do the fix in the modal component?
scruffian
approved these changes
Jun 12, 2023
16 tasks
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…wse Mode Sidebar (WordPress#50880) * added more menu to navigation with delete menu option * modal for the rename function * prop refactoring, title on rename popup * very ugly renaming of the menu * duplicate function * added snackbar notices to each action * navigate to correct places on delete and duplicate. Added Copy after title on duplication * i18n Co-authored-by: Ben Dwyer <[email protected]> * Another i18n Co-authored-by: Ben Dwyer <[email protected]> * Apply suggestions from code review Co-authored-by: Ben Dwyer <[email protected]> * Use self documenting var name * Use consistent confirmatory action wording * Refactor MoreMenu to component * Extract Modal to seperate component * Refactor change function * Use local state and only edit record on “save” confirmation * Localise state closer to where it’s needed * Disable “Save” button until menu title is modified * Remove file import introduced during rebase * Handle Save errors * Error handling for Delete action * Force all actions to throw on error * Add error handling for Duplicate. Also pass error message and not error object. * Improve loading feedback when saving or deleting * Make prop naming consistent * i18n of modal title * Add confirmatory step prior to delete action * Fix rename input focus border clipping * Use default variant to get correct styles * Make `Copy` translatable * Updating confirmatory wording --------- Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Ben Dwyer <[email protected]>
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.
What?
Following the UX suggested in #50583 this PR implements a new ellipsis menu in browse mode that allows us to rename, duplicate and delete menus
Closes #50583
Why?
Some of these actions were not available to the user yet.
This is part of the effort to improve Navigation on browse mode.This PR has subsequently undergone several revisions and so it should be in a good state.
How?
Follow Ups
The following items would be good for a followup.
Navigationat the top of the panel when something is changing/navigationand then reverting on error.Testing Instructions
Screenshots or screencast
Screen.Capture.on.2023-06-09.at.14-20-17.mp4
Co-authored-by: Dave Smith [email protected]