This is a follow-up to #3551183: Admin Toolbar Search: Code clean-up and refactoring of module file, to separate the changes related with CSS from the ones with PHP.

Problem/Motivation

With more robust tests being implemented in other issues, we should now be able to proceed with some code clean-up and refactoring of the admin_toolbar_search module.

Proposed resolution

Refactor files:

admin_toolbar_search/css/admin.toolbar_search.css
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...

and move related images to the corresponding 'admin_toolbar_search' module folder.

Issue fork admin_toolbar-3551724

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

dydave’s picture

dydave commented
Component: Code » User interface
Status: Active » Needs review

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 and CSS.

All the changes detailed in the issue summary have been implemented and described in the merge request MR !177 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!

ressa made their first commit to this issue’s fork.

ressa’s picture

ressa
he/him
commented

Great work here @dydave! I had to read up on nested CSS, but it works well. And very nice with changing to rem, as well as all the comments, it helps a lot with the general understanding of what's going on.

I only have a few suggested changes, but instead of starting a review I made them directly in the file, and I hope it's all right?

The test error seems unrelated.

Maybe you can review the CSS changes? (Then I won't change the Status back and forth) Thanks!

  • dydave committed 2ee24d5e on 3.x 
    Issue #3551724 by dydave, ressa: Admin Toolbar Search: Clean-up and...
dydave’s picture

dydave commented
Status: Needs review » Fixed

Great work @ressa!🙏
Thanks a lot once again, for all the great help!

I reviewed and tested carefully your changes locally, after rebasing the merge request with the latest changes and they seemed to work great! 👍

I'm not sure exactly why the Gitlab CI Stylelint job does not catch the same errors I see locally, when developing with VS Code....
It seems much more lenient on GitlabCI or maybe we're missing some kind of configuration... 🤔
(to raise the validation level, or something like that?)

Very good catch though on fixing any Stylelint issues ==> I'll make sure to do the same locally, even though they are not raised in the Stylelint validation jobs.

After doing a quick review and another round of manual tests locally, I went ahead and merged the changes above at #6. 🥳

Marking issue as Fixed, for now.

Let's keep up the good work with the refactoring of Admin Toolbar Search! 👍
See: #3551183: Admin Toolbar Search: Code clean-up and refactoring of module file which also fixes and cleans-up the Search Shortcut feature.

Thanks again for the additional changes, reviews and testing! 🙂

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

You're welcome @dydave! And thanks for a thorough solution in the first place, and this follow up with a detailed description of your considerations.

I probably ran Stylelint to check if my changes were all right, because the test before my changes were also green. But the Gitlab CI Stylelint set up may be set up to be more lenient, for example to not fail, if the order is not absolutely correct? Like eslint test does now, where it seems to be set to "allowed to fail".

¯\_(ツ)_/¯

Though raising the validation level, can be considered, to also include wrong element order, it may result in an overwhelming amount of errors 😅

But interesting that VS Code seems to be more lenient ... perhaps you can give Stylelint in DDEV a try, and see if you get more feedback, if the order is wrong? And by the way, you can probably get a free PhpStorm license, if you're interested, https://www.drupal.org/drupalorg/blog/drupal-association-technology-part....

And yes, let's continue with the task of getting Admin Toolbar Search in better shape!

Status: Fixed » Closed (fixed)

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