Problem/Motivation

Following the recent rounds of refactoring of the Admin Toolbar Search code base, in particular with the development of additional FunctionJavascript tests, the last step before being able to create a new stable release of the module, is the refactoring of module's JavaScript code: admin_toolbar_search.js.

Mostly, the idea behind this is to take the opportunity of fixing ESLINT validation errors to rewrite the code using Vanilla JS, as much as possible and rely on the existing automated tests base.

Additionally, we could try improving the existing logic and fix issues such as

The JS code of the Admin Toolbar Search module could use a global round of clean-up, while trying to retain certain features of the existing logic.

Steps to reproduce

See ESLINT build validation errors on 3.x:
https://git.drupalcode.org/project/admin_toolbar/-/jobs/7696846#L48

Proposed resolution

  • Fix all reported errors and refactor JS code to use Vanilla JavaScript: Replace JQuery calls where possible with native JavaScript code.
  • Show the display text of the suggestion, in the search field input, instead of the link URL (see image below).

Remaining tasks

User interface changes

The search selection should now select the display text of the suggestion, in the search field input, instead of the link URL currently, see below: Using the keyboard "up" or "down" arrows.

Before:

After:

API changes

Data model changes

CommentFileSizeAuthor
#7 admin_toolbar_search-refactor-js-after2a.jpg55.33 KBdydave
#4 1.2-mr-double-display-name.png26.94 KBressa
#4 1.1-currently-one-display-name.png25.21 KBressa
admin_toolbar_search-refactor-js-after1a.jpg74.78 KBdydave
admin_toolbar_search-refactor-js-before1a.jpg104.61 KBdydave

Issue fork admin_toolbar-3564229

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 !193

dydave’s picture

dydave commented
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3557278: Gin theme: Improve compatibility support, +#3556898: Admin toolbar tools: Improve support for core Navigation

Quick follow-up on this issue:

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

All ESLINT reported errors for the 'admin_toolbar_search.js' file have been fixed, see job:
https://git.drupalcode.org/project/admin_toolbar/-/jobs/7734585#L84
The last reported file to be fixed: 'js/admin_toolbar.js'

Questionable features:

This is a complete rewrite with Vanilla JS of the original JS code, trying to retain certain undocumented pieces of logic, that could be questionable, for example:

 

Overall, I tried refraining from making Functional changes, as much as possible and tried to port/convert the existing logic so the module could stay ISO as much as possible.
But I wanted to report these two pieces of code, since I was really tempted of getting them removed straight 😅

If they are to be kept, they should be tested automatically and better documented ==> To be discussed in follow-up specific issues.
 

Otherwise, this change further introduces the possibility of decoupling the Menu Links search feature from the Admin Toolbar module and provide better integration with newer navigation components:

Future possible improvements:

 

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.

This should be another big step towards getting the module ready for the upcoming release. 🥳

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
Issue summary: View changes
Status: Needs review » Needs work
StatusFileSize
new 1.1-currently-one-display-name.png25.21 KB
new 1.2-mr-double-display-name.png26.94 KB

Thanks for another great job on improving Admin Toolbar @dydave!

Questionable features

About the questionable features, the risk could be that someone is using the "hide certain menu items in the breadcrumbs" with CSS feature, so it might be premature to just remove it ... Perhaps create a follow up issue for this?

About excluding the Front page, it seems like searching for "front" offers you /admin/index but not the "real" front page. Maybe that was the thought behind this code -- to only offer the admin front page ("/admin/index") but not real front page ("/") to admin users? My 2 cents: I would be fine with including the real front page ("/") in search results :) So it could be removed, or alternatively taken care of in a follow up issue?

Having the title in the input field rather than the URL is much better, so a great little by product of the update!

I did catch a single problem, which is that the titles of pages in the result list are shown twice ("Manage display > Manage display"):

Currently, page titles shown once
Currently, once

MR, page titles are shown twice
MR, twice

But besides that, the patch applies well, and search works fine, just as before (with the URL to Title improvement) and there are no errors in the Console. So thanks for yet another improvement @dydave!

dydave’s picture

dydave commented

Thanks a lot @ressa for the speedy reply and taking the time to test this carefully! 🙏

Just a very quick reply to let you know I'm able to reproduce the issue with the duplicate titles, good catch! 👍
Not sure how I could have missed that ... 😅

Working on a fix already: Should be able to update the MR shortly. 👌

I'll come back to the other points, when this small bug is fixed.

Thanks again! 😊

dydave’s picture

dydave commented
Status: Needs work » Needs review

OK @ressa, I think I found where the problem came from:
A wrong init of the breadcrumbs used to build the search suggestions display label. 👌

Could you please try giving this updated version another quick round of tests?

In the meantime, I'll take a closer look at the other comments 😊
Thanks in advance, once again, for your feedback! 🤞

dydave’s picture

dydave commented
StatusFileSize
new admin_toolbar_search-refactor-js-after2a.jpg55.33 KB

OK, for:

Excluding the Front page:
Try commenting the 3 following lines in the admin_toolbar_search.js file:
https://git.drupalcode.org/project/admin_toolbar/-/blob/66c621860088b3a1...

                    // Exclude links with URLs matching the excluded paths.
                    if (excludedPaths.includes(element.href)) {
                      return;
                    }

 
This should be the result:

Compared to the current version, I really don't see any problem of having this link in the results....
Really not sure what the initial requirements could have been 🤔

I think we should probably remove this piece of code as well and just keep the Front page link in results 👌
This way, there should not be any more custom logic related to the links selected in the admin toolbar.
 

For the other one, with the class .admin-toolbar-search-ignore, I completely agree... 👍
Let's keep this as it is for now and discuss in more details in a separate issue 😅

Let me know what you think about the Front page and I'll be happy to the add this change to the MR before getting it merged.
Thanks in advance! 🙏

ressa’s picture

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

You're welcome, and thank you for a speedy response as well @dydave, it's great to keep the momentum!

Honestly, I almost also missed the duplicate page titles, and was ready to hit RTBC, when I wanted to check something else quickly, and noticed it 😅

The new MR you created so fast works perfectly, and page titles are only shown once, as expected.

Thanks for the tip about the Front page lines. I tried it, and I prefer to include the "Front page" ("/") in the results, and vote for removing those three lines. If it simplifies things, even better! This looks good to go, thanks once again!

  • dydave committed 40fc5b84 on 3.x 
    Issue #3564229 by dydave: Admin Toolbar Search: Refactored JS code with...
dydave’s picture

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

Thanks so much @ressa, once again, for the great feedback!

Sorry for the late follow-up on this, but I got a bit side tracked with the end of the year holidays, Christmas and all! 😅☃️🎄🦌
I'd like to take the opportunity to wish you a Merry Christmas and hope you had a good time as well! 🥳

Alright!!! 👍
Another big task out of the way, with the ESLINT fixes + Vanilla JS conversion of the largest JS piece of code in module's repository 🤩

I went ahead and merged the changes above at #9, then realized I forgot adding the changes for the Front page logic 🤦‍♂️
Never mind... I'll add these changes to one of the other tickets for automated tests 👌

For the other one, with the class .admin-toolbar-search-ignore, I created the follow-up task:
#3564883: AT Search: Improve tests and documentation for admin_toolbar_search.js

I've added the improvements suggestions to the same ticket and also in the Gin theme compatibility one, for now.
We'll probably need to do more testing with the Gin theme and core Navigation module to see how it could potentially interact with the admin_toolbar module.
At some point, we might have to remove the dependency on the toolbar module, so the AT Search could potentially just work with Gin and/or core Navigation: Exactly what we did for the admin_toolbar_tools module in #3556081: Admin Toolbar Tools: Remove dependency on Admin Toolbar.

But all these points should most likely be approached in different specific tickets:
A lot of work has already been completed in this issue, so let's mark it as Fixed for now.

Looks like we're down to the last issues for this release, with the automated tests 👍
So I "think" I should be able to take it from here @ressa, up until the new stable release is created.
I'll keep you updated in the Roadmap ticket and you'll probably see directly when the new patch version is released.

Once again, thanks a lot for your great help testing and reviewing these changes! 🙏
Let's get this big batch of tickets over the line... 🥳

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

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

Maintainers, credit people who helped resolve this issue.

ressa’s picture

ressa
he/him
commented

Merry Christmas @dydave to you as well! 🎄
And no problem at all, I think you are always very quick to handle the Admin Toolbar issues. And getting the JavaScript in tip top shape with your huge efforts is amazing! Thanks for creating a follow-up issue for the 'admin-toolbar-search-ignore' feature, it's great to deal with left overs, which could perhaps get cleaned out.

It sounds like a great plan to detach Admin Toolbar Search from the Admin Toolbar module, for maximum flexibility with other modules, and interaction with them. And great that almost all issues are now done, and the next release might be close. I'll follow along in the meta issue, thank you so much @dydave!

Status: Fixed » Closed (fixed)

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