Problem/Motivation

The module does not support Drupal 8 anymore, so it does not seem necessary to keep a specific Tools menu toolbar icon logo for Drupal 8:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...

It seems this feature was introduced in issue:
#3158374: Show Drupal Icon depending on which drupal major version it is.
corresponding to commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/46ca3837f2e118...

There does not seem to be specific logo images for Drupal 9, 10 or 11, which all currently use the same D9 logo:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...

I've tried searching for an equivalent logo for D10 or D11 and it would seem the D9 logo has now become the "standard"....
All the D10 or D11 logos I could find would just reuse the D9 logo with a '10' or '11' written in text, next to it...

Since there has not been any request or complaint to use a more specific logo for D10 or D11, maybe we could assume this feature might not be used anymore.

Steps to reproduce

Install the admin_toolbar_tools module with D9, D10 or D11 and check the logo image of the Tools menu in the toolbar is always the same.

Proposed resolution

Two options are possible:

  1. Either a new logo is found, suggested and used for more recent Drupal core versions and will have to be updated for each new version.
  2. Or, remove this feature for the time being.
    If a new logo for a new version is found or another image needs to be used, we should be able to create another issue.

For the time being, I would personally recommend we go with the second option: Removing this feature, always leaving us the possibility to come back to it at a later stage, when it becomes needed again.

Expected changes:

  • Create a revert commit based on the initial commit for this feature to remove it.
  • Take the opportunity to standardize the name of the CSS file from tools.css to admin_toolbar_tools.css.
  • Clean-up: Try refactoring and commenting a bit the CSS code of the file.

Feel free to let us know if you would have questions or suggestions on this issue or a potential logo, we would surely be glad to hear your opinion and feedback.
Thank in advance!

Issue fork admin_toolbar-3550270

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

dydave’s picture

dydave commented

Quick follow-up on this issue:

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

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

Overall, this merge request is really just a "routine" maintenance task trying to clean-up some old code and unused CSS rules.

For the time being, I would personally recommend moving forward with this merge request and removing any code that would not be effectively used in the module, for the core versions currently supported (YAGNI!).

Less code means fewer automated tests, a slightly lighter/faster CSS and module, and of course less maintenance work 🙂

Otherwise, if anybody has a better idea on which logo files could be used, we would be glad to hear it 👌

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!

dydave’s picture

dydave commented
Status: Active » Needs review
ressa’s picture

ressa
he/him
commented
Status: Needs review » Needs work

Thanks @dydave, great initiative, always nice to get rid of unneeded code! I agree with your considerations, and support removing the version switch function, and just use what seems to be the de facto Drupal logo, for Drupal 9, 10, 11 ...

I tried the MR, and it works perfectly. The old images are replaced with a single drupal-logo.svg, and the update hook flushed caches (nice touch!) and the icon was immediately available.

I only have a single comment about hook update numbering:

Hook update numbering
It looks like update hooks numbering diverged at some point ... 

$ grep -rinoE '.{0,20}update_8.{0,20}' .
./admin_toolbar.install:13:ction admin_toolbar_update_8001() {
./admin_toolbar.install:23:ction admin_toolbar_update_8002() {
./admin_toolbar.install:37:ction admin_toolbar_update_8003() {
./admin_toolbar.install:48:ction admin_toolbar_update_8004() {
./admin_toolbar.install:84:ction admin_toolbar_update_8005() {
./admin_toolbar_search/admin_toolbar_search.install:11:dmin_toolbar_search_update_8001() {
./admin_toolbar_tools/admin_toolbar_tools.install:11:admin_toolbar_tools_update_8001() {
./admin_toolbar_tools/admin_toolbar_tools.install:19:admin_toolbar_tools_update_8201() {
./admin_toolbar_tools/admin_toolbar_tools.install:29:admin_toolbar_tools_update_8202() {

I looked in the update hooks history, and it looks like 8003 should have been 8203:

30 Jan 2021: function admin_toolbar_tools_update_8202() {
https://git.drupalcode.org/project/admin_toolbar/-/blame/3.x/admin_toolb...

2 Aug 2024: function admin_toolbar_update_8003() {
https://git.drupalcode.org/project/admin_toolbar/-/blame/3.x/admin_toolb...

An update_hook should always be higher than the previous one:

[... ] what matters is that you must ALWAYS increase the number when a new update hook is added.

From https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

... so we could either carry on with 8203 (which could be seen as a bit strange, since it's based on 8.2 ...) or use the current versioning. Since it should be higher it could be 37001, since the release will be 3.7.0 and then a "0" and a "1" = 37001 ... what do you think?

dydave’s picture

dydave commented
Status: Needs work » Needs review

Super good catch @ressa!
Once again! Thanks a lot! 🙏

Totally.... That's my bad! 😖
I think it comes from a copy/paste from another MR or module (admin_toolbar_search, I think 😅)...

so we could either carry on with 8203 (which could be seen as a bit strange, since it's based on 8.2 ...) or use the current versioning. Since it should be higher it could be 37001, since the release will be 3.7.0 and then a "0" and a "1" = 37001 ... what do you think?

You're completely right we should probably consider changing the standard....
But I'm not sure we would really be releasing this batch of tickets in a 3.7.0, perhaps 3.6.3?
At this point, I don't think we've made any Backward Compatibility (BC) breaking changes, except maybe, translations strings, which is probably "minor" considering the strings in this module 👌

I was thinking about this the other day and we're not making config changes, API compatibility changes, etc...
I think it's fine to keep using the same Meta Roadmap ticket, even though we don't release a new minor:
My thinking behind that is that it would probably be much easier for sites to update for a patch version rather than minor....

Since we're still unsure for now, I would suggest maybe keeping the same pattern (first option you pointed out), or maybe using the current minor: 36201?

For the time being, I updated the MR with the correct numbering 8203 following the existing standard.
(Just fixed the wrong copy/paste)

Moving back to Needs review, let me know if you still think we should update the version numbering in the MR, I would be glad to make the changes 🙂
Otherwise, we could still do it in future merge requests 👌

In any case the update hooks should work fine 👍

Thanks in advance!

ressa’s picture

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

Great work @dydave!

And no problem with the version, the mix up of versions happened a while ago, and it's only great to get it corrected now 🙂

About the update version, I just picked 37001, since that's the one used in the #3525938: [Meta] Roadplan for Admin Toolbar 3.6.3 issue. But using 8203 is just perfect, and least disruptive as a pragmatic solution. And maybe the version in the Meta issue title should be updated? I'll let that be up to you. Thanks!

PS. Just a detail, but for fun, I ran the Stylelint on the CSS file, and got a few "wrong order" errors.
Interestingly, Stylelint is green in the test: https://git.drupalcode.org/project/admin_toolbar/-/jobs/6968957

user@drupal10-web:/var/www/html/web/core$ npx stylelint "../modules/contrib/admin_toolbar/admin_toolbar_tools/css/admin_toolbar_tools.css"
../modules/contrib/admin_toolbar/admin_toolbar_tools/css/admin_toolbar_tools.css
  12:5  ✖  Expected "padding" to come before "background-image"    order/properties-order
  14:5  ✖  Expected "margin-left" to come before "padding-bottom"  order/properties-order
✖ 2 problems (2 errors, 0 warnings)

This order seemed to be the right one, it should be safe to add it without review, don't you think?

/* Replace the menu label text with the Drupal logo as an icon. */
&::before {
  box-sizing: content-box;
  margin-left: 0.25rem;
  padding: 0.125rem;
  padding-bottom: 0;
  background-image: url(../images/drupal-logo.svg);
}

  • dydave committed ed157259 on 3.x 
    Issue #3550270 by dydave: Admin Toolbar Tools: Clean-up and refactoring...
dydave’s picture

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

Thanks a lot @ressa for the CSS code! 🙏

I could see the Stylelint order errors in my VS Code editor and after pasting the code from your last comment (#7), they disappeared. 👍

I then did another quick round of manual testing locally and since everything seemed to work fine, I went ahead and merged the changes above at #8.

That's great progress so far @ressa! 🥳

We've got:
✅ 2 images removed from the repo \o/
✅ 2/2 sub-modules CSS files fully refactored (cleaned-up, simplified and documented).

Probably the next big piece of refactoring is the PHP code: 😅
#3551183: Admin Toolbar Search: Code clean-up and refactoring of module file
with a few bug fixes as well, in particular:
#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.

Which introduces a new search field title attribute string, when the search shortcut is disabled:
Type text to search in menu links in the admin toolbar.

Let's keep up the efforts with the next tickets, looking forward to your feedback! 🤞

Marking issue as Fixed, for now.

Thanks again @ressa! 🙂

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

You're welcome @dydave, thanks for yet another fast response and commit. Great that obsolete images were removed and CSS files refactored!

Status: Fixed » Closed (fixed)

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