Improves the UX of menu management in the navigation block #42987
Conversation
|
Size Change: +487 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
Status:
I may be using the wrong component. |
draganescu
changed the title
|
Reminds me of #18202 😄 edit: just for reference, that was removed in #23022 (issue #20846, also see #19691). Something not captured in that issue, but I remember was an issue is that when there are a lot of menu items, the sidebar content can be quite lengthy. Other block tools get pushed out of view and a lot of scrolling is required. |
scruffian
force-pushed
the
try/navigation-list-view-block-controls
branch
from
c1531ca
to
11f18ce
Compare
draganescu
force-pushed
the
try/navigation-list-view-block-controls
branch
from
11f18ce
to
d0872b3
Compare
|
Maybe for ease of review I'll create a separate PR adding the list view back. It also seems that the inspector of this block is getting extremely cluttered. |
draganescu
force-pushed
the
try/navigation-list-view-block-controls
branch
from
be27bc7
to
0009541
Compare
|
This is a much better UX, I agree. It makes more sense to me to handle this in the sidebar. It's working as intended for me |
|
I found a |
There was a problem hiding this comment.
Thanks for your work here. I believe the goal of reducing the UX "noise" currently associated with the block is a good one.
I noticed a few issues that I think we should look to resolve prior merging this.
The menu selector is lacking a border.
Whilst loading the Menus section is empty and then changes to "Create new menu" even though menus are ultimately going to load.
The `Manage menus` link seems to disappear and reappear each time you select a Menu.
Screen.Capture.on.2022-08-26.at.10-11-46.mp4
The UX when converting a classic menu doesn't provide good feedback. The Menus panel still shows the old menu and then suddenly updates. This leaves me able to change menus or even create new ones whilst the conversion process is happening. We need to improve the guards and gates using the data exposed by the conversion process utility hook.
Screen.Capture.on.2022-08-26.at.10-13-15.mp4
Similar problem with `Create Menu` action. It might be good to merge [#43529](https://github.com//pull/43529) (open with no reviews 😢 ) and _then_ look to take the learnings forward into this PR.
Screen.Capture.on.2022-08-26.at.10-14-59.mp4
Overall this is heading in a good direction. I think we need to do a bit more work to tidy up the UX to ensure we're providing solid feedback to the user.
One thing I would recommend trying that helped me was to test this using a slow internet connect as that quickly uncovers issues that can be masked by using a fast connection. I hope that helps?
|
Great review @getdave ! 😬 all these behaviors were exposed by the moving of the control as the panel in the inspector does not disappear but the toolbar section does and the effect is more jarring. Looking into it. To do:
|
draganescu
force-pushed
the
try/navigation-list-view-block-controls
branch
from
0816e4a
to
63ddb37
Compare
|
This PR will likely resolve/invalidate #38169 as well |
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
draganescu
force-pushed
the
try/navigation-list-view-block-controls
branch
2 times, most recently
from
b0bc044
to
50d69e2
Compare
|
I have updated the PR description with a video with the current behavior. I'd love some design feedback at this point as the UI exemplified in #42602 (comment) is covered is covered but raises a few points to address:
This PR should not attempt to solve the general behavior of the Navigation block and how entities are loaded for it, as this causes most of the flickering / appearing / disappearing. For example, I think some of the things (like users perms) should be saved until page reload (between renders) - but then this PR is already a soup of refactoring and new functionality, I'd love to not make it worse. |
…so we refresh this quite often when switching between menus. For a more consistent UI experience we disabe the menu management link untill `hasManagePermissions` is refreshed. Another option would be to cache this at a component level. But, this could be more accurate?
Fixes slow connection UX by disabling and enabling the selector and the manage menus link depending on data status. Co-authored-by: Dave Smith <444434+getdave@users.noreply.github.com>
draganescu
force-pushed
the
try/navigation-list-view-block-controls
branch
from
50d69e2
to
d641309
Compare
|
Thank you for working on this @draganescu Andrei! Testing. Before/AfterBefore:
After:
Should the Menu section be seen above the Layout section? As in importance it would be best to first see Menu section and then the Layout section. Should the menu drop down have a border/outline giving a stronger signal that it is a drop down? Clicking Manage menus leads one out of the page and into a Navigation Menus screen somewhere in the backend, and not connected with any place. Similar to managing Reusable blocks. Clicking to edit a menu in the Navigation Menus screen one can only edit the name and not the contents of the menu. What else should I check? My conclusion is that it is nice to add this specific feature into the sidebar. I look forward to having more of the Navigation features in the sidebar. Thanks. |
|
Thanks for testing @paaljoachim 👍🏻
Yes, but for some reason the layout options cannot be overiden yet.
What browser were you testing with? I and others cannot repro the lack of border but you're the second to observe it.
Updated the PR description with items. |
- The label of the currently selected menu updates if menu is renamed from advanced - Uses a visually hidden label to explain this is a menu switcher - Defaults to a button if there are no block menus and no classic menus - Renames "Loading options ..." to "Loading ..." Co-authored-by: Javier Arce <4933+javierarce@users.noreply.github.com> Co-authored-by: Alex Stine <13755480+alexstine@users.noreply.github.com>
|
Local site using Desktop Server. I have now tested with Brave, Firefox and Chrome. None had a grey border around it and looked like this (screenshot from Chrome):Did an Inspect and noticed the .components-button had border 0. I adjusted it to 1px solid grey. It now has a grey border around the drop down as does every other button...:) So it is not the place to add it but I did it to experiment.
Btw it took a moment (saw the spinner instead of the Nav block menu for half a second) before the menu showed up. |
I think it looks a bit like an error state, so I do think it's worth revisiting. Previously the placeholder was shown, but I guess there is no placeholder now. I see it wasn't introduced by this PR, so don't worry about it right now. |
For the sake of consistency and simplicity, we should only have one message "Select menu" (and obviously "Loading..." for the loading state). Having different messages is confusing and could cause problems in other languages. So we would have this:
Right, we should show "Select menu" because there are menus (the only difference is that the user will be able to import and select them simultaneously).
Gotcha, thanks for the explanation! |
There was a problem hiding this comment.
I tested this and it's really good. I especially appreciate the work to ensure that the select menus close immediately upon selecting an option that causes an async action to occur.
Missing border appears to be coming from `.components-button`.
I noticed that when switching between menus that are similar it's not always obvious that something has happened. We might need a follow up to announce (via a notification) that the menu has changed - especially for users of assistive technology.
Screen.Capture.on.2022-09-05.at.10-14-30.mp4
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
@draganescu I was testing with TT2. Now I'm on `emptytheme` the border is there 🤔
|
- replaces disabled with isBusy for toggle working state - refactores some conditionals for better readability - adds consistency to the toggle label Co-authored-by: Daniel Richards <677833+talldan@users.noreply.github.com> Co-authored-by: Dave Smith <444434+getdave@users.noreply.github.com> Co-authored-by: Javier Arce <4933+javierarce@users.noreply.github.com> Co-authored-by: Paal Joachim Romdahl <5323259+paaljoachim@users.noreply.github.com>
|
This is looking great, but there's a small detail we could fix: if the user opens the dropdown and clicks on the already selected menu, the dropdown doesn't get closed. The proper behavior should be to close the dropdown. |
|
@getdave it seems that after @scruffian 's commit moving the control styles from @javierarce we specifically implemented in the chances from @talldan that I brought over from #42956 to not close the menu when switching for a11y reasons. @alexstine if we close the dropdown when selecting the same menu, should we focus the block or the control? Either way this can be revisited. @talldan Some details on The I'll leave it as is, as multiple clicks don't damage data, it can only cause buggy UI and will come back with some kind of update for This is the second issue I run into with a component in this PR - the 1st one being with |
|
I will merge this and revisit the following:
|
@draganescu I'm not sure I follow. I think it was a normal I guess it may also happen with dropdown menus and that's what you're referring to. Anyway, I looked a bit deeper and I think I wasn't completely correct with my previous advice. You can use <Button isBusy={ isCreatingMenu } disabled={ isCreatingMenu } __experimentalIsFocusable>Create menu</Button>edit: gutenberg/packages/components/src/button/index.js Lines 119 to 130 in ec29b9c |
|
@draganescu It doesn't look like inspector menu editing made it in this round. It still seems valuable, is there any chance we can still land it? Edit: from the context it looks like it was an oversight. I've reopened! 👍 👍 |
What?
Closes #42257, #42602, #38169
Why?
Simplifying the UX of the navigation block. The crowding of the block toolbar with all kinds of disconnected actions does not appear as a good path.
How?
A few tweaks:
Optional:
To do
isBusyTesting Instructions
Screenshots or screencast
menu-switcher-gprs.mp4