- 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.