Problem/Motivation

This issue is a follow-up to #3550604: Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks where different display orders were identified for some of the extra links added by the Admin Toolbar Tools module.

For all entity types, the order of the extra links menu items is:
1 - All types
2 - Add type

Entity operations links:
1 - ... Other links ...
2 - Devel
3 - Delete

In general, throughout the admin_toolbar_tools module, the "All types" link is always displayed first and the "Delete" link is always displayed last.

This is not currently the case for menus and user roles which have the following display order:
Menus:
1 - Add menu
2 - All menus

and menu operations links:
1 - Add link
2 - Delete
3 - Devel

User roles operations links:
1 - Delete
2 - Devel
3 - Edit permissions

Standardizing the display order would not only simplify the maintenance and the understanding of module's code and logic, but would also be more consistent for users of the module:
The links would all be in the same "place" (order) for all the different entity types (block content, taxonomy terms, user roles, menus, etc...).

Steps to reproduce

1 - Install the Admin Toolbar Tools and Devel modules.
2 - Configure the max bundle number to 2 on the settings form page (admin/config/user-interface/admin-toolbar-tools) so the "All menus" link displays by default, without having to create anything.
3 - Check the display order of menus and user roles added extra links.

Proposed resolution

Change the menu link weight of the corresponding menu items in the ExtraLinks Plugin class:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
so they display in the correct order:

Menus:
1 - All menus
2 - Add menu

and menu operations links:
1 - Add link
2 - Devel
3 - Delete

User roles operations links:
1 - Edit permissions
2 - Devel
3 - Delete

Feel free to let us know if you have any questions or concerns on any aspects of this issue or the project in general, we would surely be glad to hear your feedback.
Thanks in advance!

CommentFileSizeAuthor
#3 admin_toolbar-user_roles-extra-links-right-order1a.jpg42.46 KBdydave
#3 admin_toolbar-menus-extra-links-right-order1a.jpg27.87 KBdydave
admin_toolbar-user_roles-extra-links-wrong-order1a.jpg42.64 KBdydave
admin_toolbar-menus-extra-links-wrong-order1a.jpg27.05 KBdydave

Issue fork admin_toolbar-3552168

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dydave created an issue. See original summary.

dydave opened merge request !179

dydave’s picture

dydave commented
Status: Active » Needs review
StatusFileSize
new admin_toolbar-menus-extra-links-right-order1a.jpg27.87 KB
new admin_toolbar-user_roles-extra-links-right-order1a.jpg42.46 KB

Quick follow-up on this issue:

I think it is better to isolate these changes so they could really be focused on testing the display order of the updated menu link items.

All the changes detailed in the issue summary have been implemented and described in the merge request MR !179 above at #2.

Resulting display order, after applying the changes from the merge request:

Menus:

User roles:

There are no tests in this merge request, since they are added in related issue #3550604: Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks.

Since all the tests and jobs still seem to be passing 🟢, moving issue to Needs review as an attempt to get more testing feedback and reviews.

Feel free to let us know if you have any comments, questions or concerns on any aspects of this issue or the suggested changes in the merge request, we would surely be glad to help.
Thanks in advance!

ressa’s picture

ressa
he/him
commented
Status: Needs review » Reviewed & tested by the community

Whew, this one took me a while to understand 😅

As a side effect I finally understood what "Maximum number of bundle sub-menus to display" does. As a follow up, it could be updated ... Perhaps drop "Bundle"?

Thanks for creating the MR @dydave, and thoroughly describing the challenge, the solution, and the expected result. The MR works as expected, and the menu items now all follow the same order:

Menu
BEFORE

  1. Add link
  2. Delete
  3. Devel

AFTER

  1. Add link
  2. Devel
  3. Delete

People
BEFORE

  1. Devel
  2. Delete
  3. Edit permissions

AFTER

  1. Edit permissions
  2. Devel
  3. Delete

Menu
BEFORE

  1. Add menu
  2. All menus

AFTER

  1. All menus
  2. Add menu

Content type
BEFORE

  1. All types
  2. Add content type

AFTER

  1. All types
  2. Add content type

  • dydave committed 5e5ced63 on 3.x 
    Issue #3552168 by dydave: Standardized display order of extra links menu...
dydave’s picture

dydave commented
Status: Reviewed & tested by the community » Fixed

Thank you so much @ressa for taking the time to wrap your head around this one 😅

Again, this is probably pretty minor, but it made things simpler for using a standard order for testing all these links for entity bundle links, see for example the 'All types' link:
https://git.drupalcode.org/project/admin_toolbar/-/blob/c5fc0f14545808fe...
which should always be first, or the 'Add type' links:
https://git.drupalcode.org/project/admin_toolbar/-/blob/c5fc0f14545808fe...

In other words, while the testing logic worked for all other entity types it didn't just for 'menu' and 'user_role' 😅
So we had the choice to either keep the existing logic and add a special case in there tests.
Or keep the same generic testing logic and standardize the order of the problematic menu extra links in the code. 👍

The Devel links are not tested automatically though, but manual testing should help confirm the changes are consistent for all supported entity types.

In any case, this merge request only impacts the 'weight' of the corresponding menu extra links, so it should be very safe to move forward with these changes. 👌

So, following your confirmation above at #4, I went ahead and merged the changes above at #5 🥳

Marking issue as Fixed, for now.

One more to go #3552177: Add 'Manage Permissions' and standardize menu label in Admin Toolbar Search suggestions and we should be able to move forward with the Functional integration tests. 👍

Thanks again very much for your great help and time testing/documenting @ressa! 🙏

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

ressa’s picture

ressa
he/him
commented

Fantastic @dydave, another fixed issue, thank you!

I agree 100%, and am all about standardizing and streamlining as much as possible, to allow the maximum amount of automation and control. Handheld solutions will eventually break, or "something" happens. And reducing the amount of code is a great win as well, for less maintenance 👍

It's great with the steady progress in the issue queue, checking them off one-by-one!

dydave’s picture

dydave commented

Thanks @ressa!!

Let's keep working our way down the stack!

There are still 5 issues to go, on which I would greatly appreciate to have your feedback and reviews!

I'm jumping in meetings, but I will have more time in the evening to answer!

Thanks you so much!! 🙂

Status: Fixed » Closed (fixed)

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