Creation of menus on the edit navigation screen #22309
Conversation
talldan
added
[Type] Enhancement
|
Size Change: +2.24 kB (0%) Total Size: 835 kB
ℹ️ View Unchanged
|
|
👋 I tested this locally and noted the following:
|
talldan
force-pushed
the
add/nav-menu-create-button
branch
from
2ab0e73 to
64716e2
Compare
|
Thanks for testing @draganescu. I'll work on validation, cancelling menu creation, and also have a fix for that error (that I'll move into a separate PR as well). The flickering is a tricky one to solve and I might have to defer to a separate PR. It seems to be related to It might be possible to use some smoke and mirrors to hide this, maybe by adding a debounce to the spinner. |
packages/edit-navigation/src/components/menus-editor/create-menu-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menus-editor/create-menu-panel.js
Show resolved
Hide resolved
talldan
force-pushed
the
add/nav-menu-create-button
branch
from
0ee88f7 to
1ccfdda
Compare
|
Not too far off with this, though there are a few issues I've outlined in my comments above. Would welcome further testing, reviews, and opinions! |
talldan
requested review from
tellthemachines,
draganescu,
noisysocks and
adamziel
|
One other functional tid bit @talldan, it would be nice that after menu creation the selected one is the newly created menu. |
| onDelete={ onDeleteMenu } | ||
| /> | ||
| </div> | ||
| { hasMenu && ( |
There was a problem hiding this comment.
Should this component just return null when hasMenu === false?
There was a problem hiding this comment.
I ended up removing that and going with the suggestion of @paaljoachim below and showing the create panel alongside other panels.
|
|
||
| return ( | ||
| <Panel className="edit-navigation-menu-editor__navigation-structure-panel"> | ||
| <PanelBody | ||
| title={ __( 'Navigation structure' ) } | ||
| initialOpen={ initialOpen } | ||
| > | ||
| { !! blocks.length && ( | ||
| { showNavigationStructure && ( |
There was a problem hiding this comment.
Same as above - should this component just return null when showNavigationStructure === false?
There was a problem hiding this comment.
I changed this back to how it was before for the same reason as above, the panel will show at the same time as the create panel.
| @@ -68,6 +68,10 @@ | |||
| -ms-grid-row-align: start; | |||
| } | |||
|
|
|||
| .edit-navigation-menu-editor__cancel-create-menu-button { | |||
| margin-left: 1ch; | |||
There was a problem hiding this comment.
It seems like we're using either 6px or 8px for this kind of margin in other places.
| return; | ||
| } | ||
|
|
||
| saveMenu( { name: menuName } ); |
There was a problem hiding this comment.
Something like this should take care of the notifications.
| saveMenu( { name: menuName } ); | |
| try { | |
| await saveMenu( { name: menuName } ); | |
| createInfoNotice( __( 'Menu created' ), { | |
| type: 'snackbar', | |
| isDismissible: true, | |
| } ); | |
| } catch (err) { | |
| createErrorNotice( __( 'Menu creation failed' ), { | |
| type: 'snackbar', | |
| isDismissible: true, | |
| } ); | |
| } |
There was a problem hiding this comment.
Thanks, that works nicely 👍
|
Aside of the things already mentioned in comments, it seems to work pretty well. Good job @talldan ! |
|
I am testing the PR at gutenberg.run. This is my initial test of the Navigation (beta) screen, and I am very impressed with all the work that has been done up to this point. Very nice! The process of creating a menu.
A new screen opens. It surprised me that a new screen was seen. To me it would seem that the accordion would be a part of the general Edit Navigation screen.
Open accordion. Another idea. Click "Create a new menu" and the accordion is seen below along with the other options. Here the user clicks "Create a new menu" and it opens in the same screen. It does not open in another "Edit Navigation" screen. User fills out the name "Top nav" and clicks "Create menu" and the "Edit Navigation" screen is auto updated. I am not saying the above is a good or bad or useful idea, but I wanted to bring up the initial brain storming that hit me. It would make the "Create menu" a hidden accordion on the same screen as the other nav items. |
talldan
force-pushed
the
add/nav-menu-create-button
branch
from
1ccfdda to
357dfda
Compare
|
I think I've fixed most of the issues now. I agree with @paaljoachim's suggestion of having the panel appear alongside the other UI, so it works similarly that now. That also inadvertently fixed the focus management issues 🎉 Also managed to fix the glitchy page updates that were occurring when creating a menu and the success/error notifications. It feels a lot smoother now. |
packages/edit-navigation/src/components/menus-editor/create-menu-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menus-editor/create-menu-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menus-editor/create-menu-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menus-editor/create-menu-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menus-editor/create-menu-panel.js
Show resolved
Hide resolved
| if ( ! hasCompletedFirstLoad && hasLoadedMenus ) { | ||
| setHasCompletedFirstLoad( true ); | ||
| } | ||
| }, [ hasCompletedFirstLoad, hasLoadedMenus ] ); |
There was a problem hiding this comment.
Nit: does it need to depend on hasCompletedFirstLoad?
There was a problem hiding this comment.
@talldan the glitchy behavior is gone, good job on this one! I left a few minor comments but nothing overly big.
The ones referring to the progress state will fix this problem where you get stuck in a progress state in some cases (but the submit button continues to work):
Also this is not specific to this PR, but I totally missed the notification, I think we should make them more visible cc @karmatosed @mapk:
Another design thing is that I think we can do better in the future with how the empty state feels right after a new menu is created:
|
I went ahead and tested at gutenberg.run again.
Inspect:
|
|
Hey @adamziel Notifications in general show up on the bottom left. Notifications become an extra visual mark in addition to an action that has just been taken by the user. In this case an extra visual mark that one has created a menu in addition to create menu action that is seen. An action should hopefully show the result of what is going on. (I can take another test at gutenberg.run again later on.) G2 Notifications issue: |
…ion fails. Co-authored-by: Adam Zielinski <[email protected]>
Co-authored-by: Adam Zielinski <[email protected]>
talldan
changed the title
|
@paaljoachim Thanks for testing. Guessing the error is related to permissions when using gutenberg.run. But the error messages should be better. It looks like the try/catch was only catching JavaScript errors, and for REST/entity errors the I've applied all the other suggestions and will merge this when tests pass. I also discovered #22404 while working on this, that can cause the created menu not to be selected if there are more than 10 menus. |
Description
Closes #21281
Allows creation of new menus from the experimental navigation menu screen.
This is fairly rough and ready, but I think good enough for a first draft.
How has this been tested?
First menu
With existing menus
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: