Add local tasks (tabs) for Admin Toolbar Settings forms

Quick follow-up on this issue:

Created initial merge request MR !139 above at #2 which adds local tasks (tabs) for Admin Toolbar Settings forms.

Moving issue to Needs review, as an attempt to get some feedback on this new Feature.

This MR "could" need some Tests for the tabs in a new test class for the admin_toolbar_search Settings form.
We could use an xpath expression for example to check for the tabs on the page (?!).

Any testing/reporting feedback, questions or suggestions would be greatly appreciated.
Thanks in advance!

I'll review this.

Hi @dydave, I've applied the provided MR as a patch, and it applied cleanly without any errors. Previously, as mentioned in the issue summary there was no way to navigate to the "Admin toolbar tools, Admin toolbar search and Admin toolbar" settings however the module configuration can be access under the menu "Configuration > User Interface > Admin Toolbar Search, Configuration > User Interface > Admin Toolbar Tools, Configuration > User Interface > Admin Toolbar".

After applying the patch, new tabs have been enabled to access/configure Admin toolbar, toolbar tools and toolbar search under Admin Toolbar Settings, i.e., /admin/config/user-interface/admin-toolbar. This is a great feature enabling users to easily switch between these sub-module settings by just switching the tabs. I've reviewed the changes and the changes are working fine.

I've reviewed the MR changes and dropped a suggestion regarding the title changes for Admin toolbar settings. Additionally, noticed one difference in the setting page title for Admin toolbar search config page. For Admin toolbar and tools config page, the page title has some inconsistency. For Admin toolbar setting config page title is 'Admin Toolbar settings', for tools it is 'Admin Toolbar Tools settings', however, for search it is 'Admin toolbar search settings'. As per my suggestion, the config page title for Admin Toolbar Search should be 'Admin Toolbar Search settings' to maintain the consistency for the modules. I've attached a screenshot for the changes to be made for Admin toolbar search config page title.

I'm keeping the issue status to Needs review for now, although the changes are working fine. If the suggested changes can be applied then we can change status to Needs work or else it be changed to RTBC.

I'm attaching the screenshots of before and after the changes of the applied MR as a patch for reference.

Thanks!

Thanks a lot Tirupati (@tirupati_singh) for the detailed review, it's greatly appreciated! 🙂

I have just updated the merge request with the following changes:

• Updated the Admin Toolbar Settings local task label, as suggested, from Settings to Toolbar settings.
• Added PHPUNIT Functional Tests to check whether the local tasks are displayed correctly.

 
I'm still wondering if it is a good idea to add tabs like that... 🤔
 
I mean, personally, I find these tabs super useful to easily switch between the forms while testing and developing on the module.
So with your positive feedback and confirmation the changes worked fine, I think we're probably going to include this new feature in the next 3.6.0 release 👌
 
In this case, we need to try to get a bit more feedback on the local tasks labels and this feature in general, so I'm going to add this issue to the 📌 [Meta] Roadplan for Admin Toolbar 3.6 Active , as an attempt to attract more attention from contributors and hopefully get more comments on this issue.
 
Feel free to give the MR another round of review and let us know if you spot anything else.
Thanks in advance!

Thanks @dydave and @tirupati_singh for creating, reviewing and improving tabs for Admin Toolbar!

(I was never sure what "local tasks" meant in Drupal -- for example in Views, "Tabs" is used, which is universally understood as being the same as a tab in a spreadsheet in Calc/Excel.)

Anyway, naming things like these menu tabs is not easy, since there is a main module, and submodules ... But I think you both reached a good balance, and the page and tab titles make sense intuitively, which is the task here.

And I agree very much with you @dydave, adding tabs is a great idea, which makes navigating between these naturally, closely related modules super easy.

Another GUI thing that I planned on addressing at some point with an issue, was the order of the three modules under "User interface", which has annoyed me for a long time. Perhaps we can take care of it, now that the UI is being worked on?

I have updated the MR, and added weight to the three modules, so that the order is more hierarchic, with "Admin Toolbar" at the top.

Before:

• Admin Toolbar Search
• Admin Toolbar Tools
• Admin Toolbar
• Shortcuts

Better?

• Admin Toolbar
• Admin Toolbar Search
• Admin Toolbar Tools
• Shortcuts

What do you think about this?

Thanks a lot @ressa for your help testing and reviewing this issue, it's greatly appreciated! 🙏

Glad to hear you find the idea of adding tabs could be a useful improvement 👍

I'm not sure exactly why the menu items would not be properly ordered by label....
Looks like by default, they are ordered by their level of dependency (?!) or something else (?!) ...
Maybe the keys of the menu links?!! I'm not really sure and didn't look further than that.

But indeed, I completely agree with you the order is currently wrong...
Looking for example at the way the config is exported, see in core.extension.yml:

  admin_toolbar: 0
  admin_toolbar_search: 0
  admin_toolbar_tools: 0

The modules seem to be correctly ordered....

Anyway, I made a quick amend to your last commit:
Increased the weights of the menu links, to add a bit more "space" between the links. Order stays the same.
Switched the Tools and Search tabs to be consistent with the order of menu links.

I'm fine with the merge request as it is, but just have one worry though about setting the weight of the menu links:
If other modules are adding links to the "User interface" menu, could there be some issues with the order the menu items would be displayed?
The links added by admin_toolbar would always be ordered first, wouldn't they?!

Maybe there could be some other way to approach this particular issue with the order of the menu items?

The changes for the local tasks shouldn't have any impact or conflict with other modules, though 👌

Thanks in advance for your help and feedback! 🙂

OK, what we "could" do, for now, is just move the Admin Toolbar menu link before the other two, so I've removed the weights for admin_toolbar_search and admin_toolbar_tools and added a very small weight difference to the admin_toolbar: -1, see commit:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/139/di...

I added the change as an extra commit so it could be reverted easily if you think it would not be reliable enough and we would still need to set a weight for other menu links.

I "think" it would still be fine to move forward with this change and come back later to other potential ordering issues with other menu items added by other modules.

Any feedback, comments or suggestions would be welcome.
Thanks in advance!

Thanks @ressa and @dydave for looking into the feedback and making the quick changes. As @ressa noticed, the menu ordering for Admin Toolbar menus links was not alphabetically and this could affect user experience somehow.

I've re-applied the MR as a patch and it applied cleanly without any errors. After applying the patch, the menu links for Admin toolbar has been sorted alphabetically as shown in the attached screenshots.

Additionally, noticed one difference in the setting page title for Admin toolbar search config page. For Admin toolbar and tools config page, the page title has some inconsistency. For Admin toolbar setting config page title is 'Admin Toolbar settings', for tools it is 'Admin Toolbar Tools settings', however, for search it is 'Admin toolbar search settings'. As per my suggestion, the config page title for Admin Toolbar Search should be 'Admin Toolbar Search settings' to maintain the consistency for the modules.

I've fixed the title label for admin toolbar search settings page from "Admin toolbar search settings" to "Admin Toolbar Search settings" as mentioned previously in the comment. I think it would be better if consistency is maintained throughout the module. I'm attaching the screenshots of before and after fixes for reference. Please review the changes.

Thanks!

Hi
I have tested MR 139 manually on Drupal version 11.x
The MR is applied successfully...

Result:
1. The sub menu order is updated
2. and sub module tabs with redirection links are updated.

RTBC+1

Thanks @tirupati_singh for fixing the case for "Admin Toolbar Search settings", that seems like the right solution :)

Thanks @dydave for looking at the weights and experimenting with a simpler approach. I think your pragmatic choice is better and less invasive, with less weight setting, letting the modules' sort themselves alphabetically, while making sure that of the three Admin Toolbar modules, the main module is always at the top.

My thoughts about having them grouped around a fairly low number, was that they would stay together, even if another module added a low weight. But setting it to -1 is great, and we can always revisit weights, if someone notices that they three Admin Toolbar modules have been separated by another module 🙂

It looks ready to me!

PS. I updated the title, since "local tasks" is so non-descriptive, and I can never remember what they mean ... I hope it's all right?

Thanks a lot everyone for the great help on this issue! 🙏

When I rebased the merge request, I didn't really pay attention and I overwrote the last commit from Tirupati (@tirupati_singh) 😅
So I re-applied the changes manually 👌
(sorry for overwriting your commit Tirupati... but the changes were not lost)

Overall, since these changes are really safe in terms of the main features of the module and all the jobs and tests were still passing 🟢, I went ahead and merged the MR above at #16. 🥳

This is going to be very practical for me when working on debugging and testing the module's settings forms 😅

We'll see later with a bit more time, if there is a better solution for the order of the module's menu links, but for now, this change shouldn't be "too" impacting (-1) 👌

Since all the work to be carried in this issue should have been completed at this point, marking issue as Fixed for now.

Feel free to let us know if you encounter any issues with any of the latest code changes, this ticket or the project in general, we would surely be glad to hear your feedback.
Thanks again everyone for the great help! 🙂

Thanks for solving yet another issue with thoroughness and attention to detail @dydave, great to get another one completed!

I was also right away missing the menu tabs, when I was trying out another MR, where I had to hover over menu items to switch between the setting pages of the three Admin Toolbar modules. So slow 🙂

Automatically closed - issue fixed for 2 weeks with no activity.