Admin Toolbar Search: Code clean-up and refactoring of module file

Quick follow-up on this issue:

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

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!

Moved all CSS related changes to a separate specific issue #3551724: Admin Toolbar Search: Code clean-up and refactoring of CSS and image files so visual changes should not be expected in this issue.

One small CSS rule had to be added to support the change to the <nolink> URL used for the toolbar item, changing the generated markup from a link <a> tag to a <span>.

Otherwise, all other changes should be focused on the YAML files and the module file.

These are the effective changes in this merge request:

• Renamed admin_toolbar_search CSS file.
• Fixed Keyboard shortcut displaying in field's title or placeholder if it is disabled.
• Replaced search toolbar tab link with <nolink> instead of the admin index URL.
• Refactored all module's HTML ID strings into a constants class AdminToolbarSearchConstants.

All of which are successfully tested in the PHPUNIT tests 🥳

Any testing feedback, comments or reviews would be greatly appreciated!
Thanks in advance!

The merge request MR !176 should also address the following bug:
#3087173-18: Allow shortcut keys to be configurable

Side note: When the search shortcut is disabled in the modules settings, the search input placeholder text still includes "(Alt+a)". Probably needs it's own issue.

These changes introduce a new search field title attribute string, when the search shortcut is disabled:
Type text to search for menu links in the admin toolbar.

The placeholder label was changed as well, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/176/di...

All Tests still seem to be passing! 🟢

Testing feedback and reviews would be greatly appreciated!
Thanks in advance!

Thanks a lot @ressa for the thorough testing and review, it's super helpful, as usual! 👍

I did find a single misstep, where it seems "Toolbar sticky behavior" > "Disabled, show on scroll-up" has stopped working, I am not sure why. I tested in latest Drupal 11.x-dev, if that makes any difference.

I did some more tests locally, with and without the big_pipe module enabled and both options seemed to work fine...
So I'm assuming there might perhaps be some edge cases, maybe with your local setup (?), but that should not be holding up this MR for now, I suppose.

If possible, could you please try doing a bit more tests, maybe with different browsers and compare with the stable (3.6.2) to see if something could have been broken in the past few commits?

We could then create another ticket specifically to fix any problem. 👌

But since the Functional tests are passing, the CSS and JS files for these options should be loading fine, so if there is a problem it could have to do with caching or the JS of the module, or other possible reasons....to be further investigated.

Also tested on simplytest.me with 11.3.x-dev and 3.x-dev and everything seemed to work fine with the sticky behavior options 🟢

Also, in expectation of the two "Remove dependency on Admin Toolbar" issues the "This module requires the following modules ..." could be replaced with "This module requires no modules outside of Drupal core." in the two README's?

Good catch, thanks @ressa!
I made the necessary changes to the README files and added them to the MR.
Honestly, this is just a quick superficial round of comments, mostly in code files, but we should definitely address the README files more specifically when we start working on module's documentation, in #3520700: Update the project page.

I recommend we try finishing the refactoring tasks first and clean-up so the next versions released could have a clearer and better identified scope of features.
For example, I don't think we should be documenting the maximum bundle number setting (admin_toolbar_tools), if we decide it should be removed in #3261664: Bundle sub-menu maximum not honored. 😅

After doing a few more tests myself locally and since all the jobs and tests were still passing 🟢, I went ahead and merged the changes above at #7. 🥳

This is a big one @ressa! 🤩
A great improvement for the Admin Toolbar Search module: the code base is getting tighter, with more comments and more tests coverage. 👍
This should be a great help with maintenance over time.

Overall, for the admin_toolbar_search module, this leaves two last files to be reviewed:

• The JS: admin_toolbar_search.js: In progress
  ==> We need to improve the FunctionalJavascript tests first.
• The SearchLinks service class: SearchLinks.php
  ==> Fix last: to be discussed how all this logic could be refactored and moved to admin_toolbar_tools.

 
Let's keep working on these issues step by step, with tests coverage, as much as possible 👌
with the next two big tickets:

• #3550652: Automated tests: Simplify admin_toolbar_search FunctionalJavascript tests
• #3550604: Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks

Which should allow further refactoring of these files. 😅

Marking this task as Fixed, for now.

Thanks again very much for your great help @ressa! 🙏