- Issue created by @anruether
- First commit to issue fork.
- 🇬🇧United Kingdom scott_euser
Not sure if this is the right approach, but this splits the menu and and expand so you can use both.
Screen recording:
- 🇬🇧United Kingdom scott_euser
scott_euser → changed the visibility of the branch 3480321-second-level-menu to hidden.
- 🇺🇸United States smustgrave
1) Drupal\Tests\navigation\Kernel\NavigationMenuBlockTest::testHtmlMarkup //li[contains(@class,'toolbar-menu__item--level-1')]/span[contains(@class, 'toolbar-button') and text()='title 3'] Failed asserting that 0 matches expected 1. /builds/issue/drupal-3480321/core/modules/navigation/tests/src/Kernel/NavigationMenuBlockTest.php:352
Verified test coverage.
Never seem grid-column used with a negative before, nice
Only moving to NW for the issue summary, as it's missing proposed solution
Since navigation is still experimental assuming don't need a CR for the twig change.
- 🇬🇧United Kingdom scott_euser
Thanks for the review! Updated the issue summary. On the -1 I can only pass credit on to stack overflow :) Front-end is definitely not my forte.
Test status - Pass
Testing steps -- Install drupal 11.x
- Login as admin
- Enable navigation
- Try to click on Structure > Display modes, which does not bring you to its menu item link /admin/structure/display-modesPatch applied successfully.
Issue looks fixed.
But, can see that the design is breaking for children menu titles.Please refer the gifs in attachments.
- 🇬🇧United Kingdom scott_euser
Looks like you need to clear your cache - both the twig template and css change. Thanks!
- 🇮🇳India sagarmohite0031
Hello scott_euser,
Test status - Pass
Patch applied successfully.
Issue looks fixed.
Testing steps -
- Install Drupal 11.x
- Login as admin
- Enable navigation
- Try to click on Configure > system,But, can see that the design is breaking for children menu titles after clearing cache.
Check attachments - 🇬🇧United Kingdom scott_euser
Thanks for checking; yes I can see the bug - I fixed it now + added more test coverage to validate.
- 🇬🇧United Kingdom scott_euser
Maybe this is tying loosely into 💬 Support Deeper Navigation Levels Active
- 🇺🇸United States smustgrave
#16 did you mean to push some changes up?
Are #15 and #13 testing the same thing btw? Re-uploading additional screenshots or clips are not needed that are same or similar are usually not needed jsut an FYI.
- 🇬🇧United Kingdom scott_euser
Ah sorry, pushed to the old closed branch! Fixed now
- 🇬🇧United Kingdom scott_euser
You can also view it at https://mr10134-y6ngslwhd9aeawkbkod3pwnb7vnmyrr3.tugboatqa.com/user/login + https://mr10134-y6ngslwhd9aeawkbkod3pwnb7vnmyrr3.tugboatqa.com/admin after logging in
- 🇬🇧United Kingdom scott_euser
Seems like random unrelated test failure (seeing that sometimes lately), rerunning
- 🇺🇸United States smustgrave
Removing summary update tag as it appears fine now
1) Drupal\Tests\navigation\Kernel\NavigationMenuBlockTest::testHtmlMarkup //li[contains(@class,'toolbar-menu__item--level-1')]/span[contains(@class, 'toolbar-button') and text()='title 3'] Failed asserting that 0 matches expected 1. /builds/issue/drupal-3480321/core/modules/navigation/tests/src/Kernel/NavigationMenuBlockTest.php:355 FAILURES! Tests: 3, Assertions: 37, Failures: 1. Exiting with EXIT_CODE=1
Shows the test coverage
Manual testing appears to have been done in #13
Code wise see nothing wrong
LGTM
- 🇷🇸Serbia finnsky
Thank you for work here.
1. We had something like this in initial versions of design. Then it was replaced with current variant. So i added required label.
2. Visual regression in focus ring https://gyazo.com/5b8989e769ebf73709d00bf94f2e0004
3. Keyboard navigation seems not worked as before. EG: Right and left arrow behaviour.
4. Also small note in MR
- 🇬🇧United Kingdom scott_euser
1. Sorry I didn't quite follow, where should I add a required label? Or is that a process FYI thing?
2. Fixed:
3. This seems to work: if you tab to the down arrow and press right/left it opens closes. I am not sure it makes sense to have both the button and link both respond to the right/left arrow? If so, do you have any tips? I tried duplicating these bits from the down arrow:
'aria-expanded': 'false', 'data-toolbar-menu-trigger': menu_level,
I might be a bit in over my head there if that is what you are expecting.
4. Changed to SDC, thanks!
- 🇬🇧United Kingdom scott_euser
In any case thank you very much for the reviewing and tips!
- 🇫🇮Finland lauriii Finland
For context, it was decided earlier that access to the second level menus would be limited in the menu. This is due to the reason that Drupal Core uses those primarily to display the same information as what would be in the navigation but as a page. As a result, users would likely develop a pattern of not accessing those pages. The UX team did research to validate that the pages aren't considered to be useful.
The tricky bit is that there are some edge cases where the second level menu is actually useful. This is not great from UX perspective, because if the user has developed a pattern of not accessing the second level pages, they would have challenges to find these pages. The original plan was to provide an alternative affordance for this use case, to make it easy to differentiate between the two use cases so that users know when to expect something useful.
What is in the MR doesn't fulfill this. I think we need some direction from @ckrina on how we should handle this scenario.
- 🇬🇧United Kingdom scott_euser
I suppose it could be in contrib extending the template and overriding the css?
- 🇬🇧United Kingdom scott_euser
If in Core it sounds like maybe we need a mechanism either to opt in or opt out of having a direct access link. If opt in would need to update a lot of a contrib projects but at least that makes it a conscious decision. Perhaps a new attribute in menu link?
- 🇫🇮Finland lauriii Finland
Core needs to provide direction for how to fix the Simple Blocks and Paragraphs examples from the issue summary. This might lead into change in either Core or these modules.
Ideally the fix for the Simple Blocks and Paragraphs would keep the Display mode use case intact because the Display mode example is as it is by design. This is where a contrib module could change the preference for the users that prefer a different behavior.
- 🇫🇮Finland lauriii Finland
If in Core it sounds like maybe we need a mechanism either to opt in or opt out of having a direct access link. If opt in would need to update a lot of a contrib projects but at least that makes it a conscious decision. Perhaps a new attribute in menu link?
We could potentially automate that; the pages we don't want to display in the navigation are always rendered by
\Drupal\system\Controller\SystemController
. It's a special controller designed to render menus. We would only want to link from the menu when the controller is something else, e.g.\Drupal\block\Controller\BlockListController
. - 🇬🇧United Kingdom scott_euser
That sounds sensible. I would be happy to work on that but sounds like we should get ckrina's opinion first? If its a go for that route, would that be okay to add to this issue's scope?
- 🇫🇮Finland lauriii Finland
@ckrina is out until next Wednesday but we could reach out to her then. I have discussed this a while back with @ckrina on a high level.
Here's the latest design I've seen for this:
Essentially in this design there would be an Overview link that would allow user to navigate to the parent link. I don't think this a finalized design so we probably want to wait for input from here before we get too far on the implementation.
- 🇬🇧United Kingdom scott_euser
Okay thanks for talking me through it! Happy to help on implementation when we get to that stage (that does look simpler to achieve)
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States michaellander
This feels like a duplicate of ✨ Add "All" or overview links to parent links Active , which feels like a duplicate of 🐛 Second level menu items can't be reached if they have children Active .
- 🇨🇦Canada m4olivei Grimsby, ON
Let’s leave this one open for the time being until we can get @ckrina's input per @lauriii's comments. As you pointed out @michaellander, this has popped up a few times, so it's a pain point for some. We should at least document the decisions made. 📌 Parent menu items with URLs are not linked to, do we make these available? Closed: outdated comes close to that, but it was from a time when the design was much less settled than it is today.
I know there have been more recent design discussions on this topic. I'm attempting to reach out to some folks that worked on the designs to see if they can lend some insight into more recent discussions. Otherwise, I agree we should get @ckrina's input here before closing it out.
- 🇬🇧United Kingdom scott_euser
To note, pages that list other things can still be useful, like /admin/structure/views is quite essential as many sites will have too many views to list in the menu + there is plenty of other useful information in that table that can help site builders choose which view edit that they lose out on if they stop being able to access it (fully aware that hiding it does not 'stop' them accessing it but certainly makes it hard to get to). I believe that would also be solved if we follow @laurii's suggestion in #30.
- 🇫🇮Finland lauriii Finland
I'm removing the Display Modes example from the issue summary since that is working by design. The remaining use cases are valid and should be addressed one way or another, either in core or by guiding how contrib modules should approach this.
- 🇨🇦Canada m4olivei Grimsby, ON
Had a chance to catch up with @ckrina today. The design that @laurii referenced in #30 is indeed the latest iteration of the design to accommodate this issue. Let’s update the MR and/or close the current MR and open a new one for this approach.
Here is a representative screenshot:
Here is a link to the Figma: https://www.figma.com/design/VCPAxetieAC2pzw7hgX3ij/Drupal-Admin-UI---De...
A couple of caveats to keep in mind:
- "Overview" is a working title for this link. Seeking some UX input on that. Other considerations might be "See all".
- The Figma designs show a fourth level ("Great Grandchild Item"). This is the subject of 💬 Support Deeper Navigation Levels Active and is out of scope here.
I've also updated the issue description to match.
- 🇬🇧United Kingdom scott_euser
Thanks for the updates!
Not a hill I'm willing to die on but FWIW...
I don't quite agree and the UX research backs that up:- e.g. version 2 (current MR) lower rate of failure, lower time to complete task
- A less clear UX article on it from NN hints at this too in item 12, but the separation only becomes clear when you actually visit the example they point to)
But I am fully aware I am late to the party and decisions have been made, so fully happy for you to ignore this; just saying my piece :)
Next steps
Before this gets work on I have a few questions:- From #30: . Actually thinking about this further:
- There is value in consistency particularly if it won't be obvious to the end user why some show overview and some not
- There is value in the overviews: some people actually might rely on the overview to understand what the options are in an area; ie, the descriptions do have value (or otherwise why are we providing descriptions in the first place). If we sometimes skip the overview we make that value inaccessible
So would be good to agree on whether we add that to scope or not (my vote is not FWIW)
- Do we just output 'Overview' and link to the parent, overriding its label effectively? Or do we make it controllable; override the menu link in some sort of pre-processing of the navigation menu links, but still allowing modules to override it back in case 'Overview' as a label is inappropriate (e.g., what if its not providing an overview?)
Then from there we can update issue summary and get to work on it (which I am happy to help with).
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
I was also around for some of those early discussions and so was going to say something similar to what @lauriii said in comment #26, so I'll try not to repeat, but I still support the decisions that were made around thsi.
Paragraphs is an interesting example, because what it does is non-standard, there are no example in core of these kind of "add" links appearing in the menu in this way, neither are there examples of all the different "types" appearing in the menu. Similarly, admin_toolbar adding links for all the Views a site has is also very non-standard. In both of these examples, if you have a lot of Paragraph types or a lot of Views, they don't all appear in the menu, so you end up going to the full page anyway. I think it's really important that we have consistent patterns for how things should be done.
I also still agree that in the vast majority of cases the Overview links are not useful, therefor adding them results in a net-negative user experience because it adds more visual noise. For screen reader users, having a lot of links that all say "Overview" is really unhelpful and could be confusing. Not just because they have to hear the word "Overview" every time the user opens a sub-menu, but also because if a screen reader user asks for a list of all links on the page, having a bunch of links that read simply "Overview" or "Add" without any other context could be very confusing.
So my feeling is that we shouldn't encourage a pattern which results in all of these generally redundant "Overview" links.
I think we can probably find a pattern that addresses what Paragraphs, Views and Admin toolbar are attempting to do, but right now I don't know what that pattern should be.
- 🇨🇦Canada joelpittet Vancouver
The split button proposed by @scott_euser in #6 🐛 Second level menu items can't be reached if they have children Active seems like the most intuitive direction here. I also share the concerns raised about the additional ‘Overview’ injected links.
@ckina and @lauriii could you give this a second look?
This affects a number of modules I have co-maintainership of such as:
- Menu Position →
- Calendar → (by way of dependency Views Templates → )
- Feeds →
And simple_block as mentioned previously and core's Display modes all under Structure I might add. Overview seems to make sense for Configuration at a glance but not Structure. Though I believe the Split button approach is more versatile.
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
The issue with split buttons, in the context of menu items, is that it introduces different interactions patterns depending on which part of the menu item the user interacts with. It can be challenging to communicate those different interactions to users and complicates the interface. These can also be challenging to use on touch devices, for users who struggle with precise mouse clicks/taps,
There's a really good article from Nielsen Norman Group that goes into much more detail as to why these should not be used for navigation:
Don’t Use Split Buttons for Navigation Menus - nngroup.com
That article also proposes some alternative option, some of which we've discussed here, like providing a link to the "landing page" inside the sub-menu, which is perfectly fine; But as we've discussed if we do this for all sub-menus, in the vast majority of cases those "Overview" links are entirely redundant.
Ultimately I don't think we're going to find a good technical solution here, and that's because this is not a technical problem, it's an information architecture problem.
In cases where the "Overview" page is relevant, because maybe that's the content type list or similar, we should simply provide that as a link in the sub-menu, and we should call that link something like "All content types". Or ultimately reorganize the entire admin menu (but that's probably out of scope here 😀). What we shouldn't do is add a lot of repeating links which use the label "Overview".
I think when we treat this as a problem of information architecture, and not a technical problem, we'll end up with a much better user experience.
- 🇨🇦Canada joelpittet Vancouver
Yes, “reorganizing the entire admin menu” is definitely out of scope! 😄 The issue largely stems from contrib modules adding their own admin pages under existing ones or creating nested subpages (but core too as screenshoted in #44 🐛 Second level menu items can't be reached if they have children Active ). Addressing this by fixing contrib would likely require more effort and education than working with the existing patterns commonly used.
The article raises valid concerns about split buttons in navigation, it might overstate their universal unsuitability. Instead of relying solely on these arguments, we should test those concerns ourselves, perhaps against @scott_euser propposal in #6 🐛 Second level menu items can't be reached if they have children Active , to see how they hold up in practice. A balanced approach that evaluates context, user needs, and new design tricks might reveal a more nuanced view of this pattern. Safe enough to try?
- Status changed to Needs work
9 days ago 10:11pm 12 December 2024 - 🇺🇸United States michaellander
We've been using the 'Overview' approach patch from ✨ Allow modules to hook into top of content/footer sections of new core navigation Active as a stop gap because users were getting so frustrated they couldn't access the 2nd level when a third level item was added. In our particular example it was a combination of the commerce → and scheduler → that was making it impossible to access the commerce dashboard.
In this particular use case I don't believe commerce should define a submenu link to 'Products' because as far as it's aware, it has no other children. However, we also can't put the onus on 'scheduler' because any number of modules could extend commerce in ways that they feel warrant adding children to the 'Products' menu item and then you have an issue with many modules determining how to keep from breaking the 2nd level 'Products' link.I'll be honest, so long as we treat the dynamic between the 1st and 2nd level different from the 2nd and 3rd level, where the 1st level is linked at the top of 2nd level, but 2nd level is not accessible at all if 3rd level exists, I believe we'll always have this confusion. I think this also includes hiding/showing behavior as related to
\Drupal\system\Controller\SystemController
. Perhaps an additional property could be added for Menu links to define an 'Override' title when children exist, but again that seems like something that should be consistent between the 1st and 2nd levels as well.It's for these reasons that I think the split button is most elegant, but I'd even take the 'Overview' solution over no solution.
- 🇩🇪Germany anruether Bonn
> In this particular use case I don't believe commerce should define a submenu link below 'Products' because as far as it's aware, it has no other children. However, we also can't put the onus on 'scheduler' because any number of modules could extend commerce in ways that they feel warrant adding children to the 'Products' menu item and then you have an issue with many modules determining how to keep from breaking the 2nd level 'Products' link.
Looks like this would also be the case with the first example from the IS, where
Simple Block
is the only child of
Block layout
.