-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Navigation: Add the navigation selector to the block toolbar #54214
Conversation
Size Change: +10.1 kB (+1%) Total Size: 1.53 MB
ℹ️ View Unchanged
|
Flaky tests detected in 7107f34. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6096127761
|
As far as I remember we specifically downgraded in UI proeminence the availability of multiple "navigations". I am curious what design feedback looks like today for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note.
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
Just for context, we've received a lot of feedback (in FSE outreach and elsewhere on PRs and Issues) that users do not know where/how to switch menus. Many users don't open the sidebar and this is causing confusion. |
icon={ moreVertical } | ||
toggleProps={ { isSmall: true } } | ||
icon={ icon } | ||
toggleProps={ toggleProps } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Augmenting toggle props with aria*
attribute to address Alex's feedback re: text label content could work here.
@@ -150,6 +152,27 @@ const MenuInspectorControls = ( props ) => { | |||
> | |||
{ __( 'Menu' ) } | |||
</Heading> | |||
<BlockControls group="inline"> | |||
<NavigationMenuSelector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about only showing this control if there is more than 1 menu? That might limit any potential confusion for users who only have a single menu (i.e. fresh install one is created by fallback algorithm).
One counter argument is that having a toolbar option that shows/hides depending on context could be confusing to users of assistive tech. Continuity of tools is probably a major factor here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also offers the import from classic option...
isManageMenusButtonDisabled | ||
} | ||
icon={ chevronDown } | ||
text={ __( 'Change' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsurprisingly in favour of this change (albeit with the caveats of the comments I left on the code itself - apologies I didn't submit those as part of this review 🤦 ).
As expressed in the Issue users have become very confused with where to find the place to change Navigation Menu.
The right hand sidebar is very hidden and many users won't know about it. Something as important as changing your menu should be prominent.
Note that this feature did originally exist, but was removed as contributors felt it was a 20% use case and risked confusing or distracting users.
I would argue that this is not the case and it's pretty common for folk to want to change menus.
I have suggested a middle ground of only showing this control if the user has more than a single menu but we should be conscious of a11y concerns around that approach.
…re than one navigation menu
…probably been deleted
I pushed an update to the labels. Also the navigation menu selector only shows if you have more than one navigation menu. |
Is there a reason we're not simply adding this to the block itself? Why not a simple select that appears when editing the block? Seems like the toolbar and sidebar are always used for controls, interested as to why they are never UI elements in the |
The block is never in a placeholder state, since we now have started creating a fallback navigation when you use the block. Also I think this action is something users might want to do after they have made changes to the block, so it would need to be accessible outside of the placeholder. |
@scruffian I am thinking about when the block is currently selected? E.g. the |
I'm a bit hesitant. Is it often you want to change the navigation source? Or is it just once, when the block is added? |
This is a good question - thank you. I don't think it's necessarily when the block is added. Most Themes, for example, come with a Nav block already in the "header" and this will have a Navigation Menu auto-selected by way of the fallback algorithm. Thus the most likely use case is a user who sees a given Navigation selected and wishes to change it to another Navigation which they previously created. Note that the fallback does it's best to pick the "most appropriate" Navigation, but this may not always be the one you expected/wanted. That's simply a facet of having an "automated" system. Also consider that we've been repeatly optimising for a linear flow whereby when the block is added we don't ask a user to make any upfront choices. Rather the flow is optimised for the block to "just work". However, if we've optimise for this route we need to improve the UI for the group of users who want to configure their Navigation after the fact. Currently I do not believe the block does a good enough job in this regard. This is somewhat supported by the feedback we've received to that effect. There has been a lot of feedback that this menu switching control is too hard to find. We don't necessarily have to add it into the block toolbar, but it needs to be given more prominence than it is now (witness that it is hidden in a sidebar under an options menu). Whether it's a less common action or not, I believe it is sufficiently important/common to warrant some extra prominence. If we're unsure about the block toolbar then what other options could Design suggest that might help to address this issue? |
#37068 is from December 2021, before the 6.3 site editor interface was created to solve exactly menu management issues. I would go back to the original issue and ask whether it was really relevant. Mainly, the proliferation of multiple menus, and subsequently the need to switch between them and delete them, it can be argued this was due to a lack of a central management location. To that end, I would still think the inspector is the right place for it. Though if need be, we could adapt this interface a bit: The "Menu" on the left could be the title of the menu, and have a chevron right next to it, and the ellipsis on the right could have just "delete menu" and "create new menu", potentially. We still need to handle the fallback behavior in a better way, right now I can only imagine that this error message itself is exacerbating things: IMO it should not be the primary action to create a new menu when an existing one was deleted. Ideally it should fall back invisibly, mark the document dirty if need be, and inform of the bugfix in the multi entity saving dialog. But anything would be better than what we have there, even a promt to choose your fallback. |
If we could de-couple:
...that would make a big difference and also allow us to address some other UX issues with the block. If I understand correctly you are suggesting the word Then the options menu becomes contextual options for the currently selected_ menu. Did I get that right? |
Right, it's a dropdown, and the panel title at the same time. It's an early Monday and jetlagged thought, so it's entirely possible I'm missing important nuance, though! |
Iirc there used to be a menu selector immediately below that "Menu" heading in the Inspector. That might be a nice middle ground that doesn't involve a unique UI? |
Yep, I think we intentionally moved that to the ellipsis to reduce the prominence. This goes back to some of the higher level thoughts on optimizing for fewer menus, which most people are likely to have if we avoid proliferating them. To that end, the dropdown will take already valuable space of the inspector. That said, if it addresses the initial issue, and avoids us having to add it to the block toolbar, I'd be open to going back to this, knowing this is something that'll need to contiously be iterated regardless. |
Yeah it's a tough one to balance and I'm also keen to avoid going in circles. Perhaps "Create new" stays in the ellipsis, and the dropdown appears only when multiple menus exist? That might address the feedback about switching being too buried, without exposing anything more. |
So in summary of the above:
Please correct any inconsistencies 🙏 |
I always struggle a bit with navigation due to the relationship between the wrapping block and the menu within. In that regard I see "Menu" as a label for the dropdown (or just as a generic heading when there's only one menu). The ellipsis feels like an element of the block, not the chosen menu, so I wouldn't expect to find rename/delete actions there. "Create new menu" makes sense, and I could see "Manage menus" living there (when there are multiple). |
I felt that when you are in the "List View" tab you are clearly editing the menu items (and thus the Navigation Menu) so it would be ok to have those menu items in the ellipsis menu. But I take your point. Also, the concept of "this is your selected menu" isn't always apparent - I'm not clear if we're trying to intentionally obfuscate this from the user. Are we? Either way, we've had feedback that the following actions are not obvious enough:
You could now make an argument that the Site View This leaves only the first item as the problem - this one seems highly relevant to the specific block and we can't handle this interaction in Site View. So are we happy with the revised proposal? |
In the "Menu" tab of the Navigation block? I don't see any need to obfuscate that when there are already multiple menus. I think we want to avoid the proliferation of menus which seems to occur when the 'create new' actions are too prominent.
I think so. We might include a 'Manage menus' link alongside 'Create new', but that can be tried separately. |
Thanks for the feedback all. I tried the ideas in a new PR: #54436 |
What?
This surfaces the interface for changing the active navigation to the block toolbar.
Closes #37068
Why?
This makes it easier for users to change the currently active navigation in the block.
How?
<BlockControls>
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Notes
This needs some design feedback: