With the D8 version (#1137920-114: Fix toolbar on small screen sizes and redesign toolbar for desktop), "People" has no child items. But if you go to admin/people, "permissions" and "roles" are available as tabs, and seem like useful things to have available in the toolbar as well. There's probably other similar examples. The technical reason why it's not showing up is the way menu_tree_output() treats hidden = -1
, but we can fix / workaround that if we can come up with a clear UX rule around which tabs we want replicated in the toolbar and which ones we don't.
Comment | File | Size | Author |
---|---|---|---|
#52 | interdiff_49-52.txt | 947 bytes | vsujeetkumar |
#52 | 1811998_52.patch | 10.4 KB | vsujeetkumar |
#49 | 1811998-49.patch | 9.4 KB | Hardik_Patel_12 |
#47 | 1811998-46.patch | 10.33 KB | codersukanta |
#47 | interdiff_42-46.txt | 3.42 KB | codersukanta |
Comments
Comment #1
tkoleary CreditAttribution: tkoleary commented@effulgentsia I totally agree that there is no clear rule now for what is part of the admin IA and what is a "local task" (tab). Part of me wants to say nothing is a local task when we have a deep menu that can expose it all, but I think that is not the answer either. One example of items that are tabs now which should probably not be menu items is the themes tabs in appearance/settings.
I think I need to do some research on how other systems deal with this but my gut feeling is that there should be a bright line, ie. nothing that is a local task should also appear in the menu.
Comment #2
JohnAlbinYes, both the Appearance page and the Blocks Admin page (which is being redone) currently have tabs that are a list of all the enabled themes on the site. That makes me not want to include tabs in the navigation. Also, the admin/config/people/accounts/fields tab feels too granular for tabs.
There are counter arguments as well.
Which makes me start to think that the "best solution" would be that we do case-by-case decision on whether the tabs should be included in the menu hierarchy. :-\ Someone prove me wrong, please. Yet-another-key in hook_menu just for "show tabs in toolbar" makes me *sigh*.
Comment #3
Shyamala CreditAttribution: Shyamala commentedtagging
Comment #4
Shyamala CreditAttribution: Shyamala commentedtagging
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedMoving to the core queue and retitling for #2.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedShortening title for mobile friendliness.
Comment #7
sunadmin_menu's rule to this is very simple:
- Default local tasks are excluded.
- All other local tasks are included.
Please note that you see many many more local tasks in admin_menu, because it implements an additional, partial subtree merge process to additionally expose individual configuration entities with their regular menu links, which would normally require dynamic request parameters to appear as tabs/tasks.
The actual/regular amount of local tasks in the system, especially when omitting default local tasks, is actually not very large.
For the horizontally oriented menu - if it would have dropdowns - that works very well.
For the vertically oriented menu I'm not sure... but I actually have fundamental usage/interface/interaction problems with that design and implementation in the first place, which likely explains why I'm not able to make sense of tasks in there.
Comment #8
Wim LeersI think #7 makes a ton of sense.
In any case, the current behavior is just plain weird and keeps tripping me up. E.g. under "Content" you see "Comments", and nothing else. But if you click on either "Content" or "Comments", you see that there also is a "Files" tab! That "Files" tab should have appeared in the menu tree rendered in the tray.
Comment #9
tkoleary CreditAttribution: tkoleary commented@wim leers
Yes! I noticed that too. Thanks for pointing it out.
Comment #10
Wim LeersComment #11
Bojhan CreditAttribution: Bojhan commentedI am in support of @sun his plan, that sounds sensible and what the vertical toolbar should have been doing in the first place (sigh, said it in the initial issue 100 times).
Comment #12
Wim LeersIs this indeed normal, or rather major?
The example I gave in #8 (which is a symptom of this issue) is a major WTF for me.
Comment #13
pixelmord CreditAttribution: pixelmord at Wunder commentedI think this is major, because it is just very confusing. -> UX
Comment #14
Bojhan CreditAttribution: Bojhan as a volunteer commentedI am not sure if this is the right issue to do that, neither that it should be bumped to major. Very few people seem to be upset by this, and the UX impact can only be solved trough loads and loads of work.
Comment #15
pixelmord CreditAttribution: pixelmord at Wunder commented@Bojhan: o.k. fair enough, but it bugs me, will create a patch to add the local tasks to the toolbar menu.
It just bugs me, so will try to solve that.
Comment #16
pixelmord CreditAttribution: pixelmord at Wunder commentedSo here's a patch that adds all local tasks for a given route to the toolbar menu.
Comment #17
lauriiiCan you please provide steps how this can be tested manually? We also need automated test coverage for this.
Unused use statements
Unused variable
Is missing docblock. Since this is a procedural function it should be renamed to _build_tasks_as_menu_tree()
Since Drupal 8 supports only PHP 5.5, new array syntax can be used like $menu_tree = [];
The right column has still space so something can be moved on the first line
Unnecessary extra line change
Before return we could add extra line change
Comment #18
pixelmord CreditAttribution: pixelmord at Wunder commentedThanks for the review, looked to fix those mentioned issues, new patch attached.
Comment #19
pixelmord CreditAttribution: pixelmord at Wunder commentedManual tests could be done as follows:
Put the toolbar in sidebar display to have the collapsible menu items shown with their children.
Without the patch you have only one sub menu item under Content (Comments). If you open the Content or Comment menu link, you see that there's another tab for a local task called files. Or with appearance you have no submenu items at all, even though there are the tasks "update" and "settings"
Applying the patch should add those local tasks to the toolbar menu as children of the main menu items. Both situations should be covered:
I will see to add automated tests
Comment #20
pixelmord CreditAttribution: pixelmord at Wunder commentedUnfortunately creating a test is hard, because the toolbar submenus are rendered via JavaScript, so I can not test for the presence of those links.
But manually testing I found a bug that the local tasks were not attached to the render array when there already was a regular submenu item.
I fixed that and attached a new patch.
Comment #21
tkoleary CreditAttribution: tkoleary at Acquia commented@pixelmord tested it on simplytest.me
Works for admin/people/permissions etc. but not:
admin/content/files
Or things like: admin/structure/block/block-content that are another level down
If we can see:
We should also see:
And while we're at it, why an admin list for form modes and view modes and not a "Display modes" page with view modes and form modes tabs?
Comment #22
pixelmord CreditAttribution: pixelmord at Wunder commented@tkoleary: You are right about the 3rd level menu items representing local tasks. I will look into that.
admin/content/files however works for me in all my tests and on simplytest.me
Comment #23
webchickSince this issue mentions admin_menu, linking a related issue talking about drop-downs.
Comment #24
pixelmord CreditAttribution: pixelmord at Wunder commentedHere's a new patch that adds also 3rd level items from local tasks.
This will add the above mentioned:
Currently I avoid adding any local tasks that would duplicate items already in the menu.
However that removes also some of the secondary local tasks , because the current approach of nesting the menu in general prevents that due to the fact that we do not repeat the default (primary) local task in the hierarchy, so then there's no parent to some of the secondary local task.
This is more an UX/UI issue. This will also get even more interesting, when we think about using a horizontal dropdown concept that is triggered by hover. For me it was always a little confusing in admin_menu that you had to remember clicking on the parent item and then getting on a page where the parent item has a "sibling" tab that would be found as a "child" in the menu hierarchy.
Example using the menu branch mentioned above:
Block layout(this is the repetition we avoid currently)Blocks(unfortunately this is a repetition of the parent route again)I personally would not mind repeating the default local task, but we might need exceptions, because we wouldn't want to do that in case no secondary tasks need to be added as children, or if it would be the only child of its parent with the same route.
Comment #25
pixelmord CreditAttribution: pixelmord at Wunder commentedNeeds review for code and usability because of the above mentioned issues with the way create the hierarchy while trying to omit the default local tasks.
Comment #27
svenryen CreditAttribution: svenryen at X-Team commentedIs any work still being done on this issue? I landed here because I could only see "Comments" under "Content" and not "Files". I must say I support fixing this.
Comment #34
nod_This need at least a reroll
Comment #35
nod_That would also help the related issue
Comment #36
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #37
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch for 9.1.x, please review.
Comment #39
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #40
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedTried to solve failure test cases.Kindly review a new patch.
Comment #42
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have tried to fix failed tests and fixed some CS issues.
Comment #44
codersukanta CreditAttribution: codersukanta at Srijan | A Material+ Company for Drupal India Association commentedComment #45
codersukanta CreditAttribution: codersukanta at Srijan | A Material+ Company for Drupal India Association commentedIt's my bad there are lots of dependency on AssertPageCacheContextsAndTagsTrait.php. So we cant modify it we need to work on the codebase.
Comment #47
codersukanta CreditAttribution: codersukanta at Srijan | A Material+ Company for Drupal India Association commentedUpdated patch
Comment #49
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedRe-rolling the patch.
Comment #50
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #52
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed tests, Please review.