Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
This is a follow-up to #3551183: Admin Toolbar Search: Code clean-up and refactoring of module file where an initial round of refactoring was done on the 'admin_toolbar_search.module' file.
Currently the module adds another toolbar item tab for supporting a mobile version when the search autocomplete is displayed in the toolbar ('display_menu_item: false').
But this results in unnecessary added code and logic which currently does not work very well, see screenshot below:
These changes were introduced in issue #3153716: New search field worse on mobile, uses second row, with commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/4065ed62009c61...
Where the initial requirement was to try to keep a more compact display of the toolbar for smaller screen sizes, thus adding another "fake" toolbar tab which could be displayed or hidden based on the screen size.
Why couldn't we just use the same mobile version provided by the core toolbar tab and tray already added when 'display_menu_item' is enabled ('TRUE')?
Steps to reproduce
To reproduce the displays from the screenshot above:
- Enable admin_toolbar and admin_toolbar_search.
- Keep the default config options.
- Resize screen for smaller devices, under '769px' ==> This should display a "Search" tab.
- Click on the 'Search' tab. Click on any other tab to illustrate the overlap situation from the screenshot above.
- Confirm the 'Search' tab stays highlighted (active) whether other tabs are selected or not.
Update the admin_toolbar_search settings at '/admin/config/user-interface/admin-toolbar-search' to enable 'display_menu_item'.
- Check the checkbox
'display_menu_item'and save the settings. The search field should now be displayed within a toolbar tray under a standard 'Search' toolbar tab (link). - Resize screen for smaller devices, under '769px' ==> This should display a "Search" tab with a standard toolbar tray, exactly as it does for all other tabs.
All the interactions between this tab and other tabs should work fine, in the standard way provided by Drupal core toolbar.
Proposed resolution
Remove the mobile tab.
Load the search tab with tray by default all the time and add some CSS rules to hide it when the search field is displayed in the toolbar.
In other words, there should be 2 autocomplete fields in the page, by default, with only a single field visible/accessible at all times, depending on the screen size.
If the option 'display_menu_item' is enabled, there should only be a single autocomplete field in the page.
This would allow moving any logic related with the responsive display to the Drupal core toolbar module and focusing any logic added by the admin_toolbar_search module on displaying the search field in the toolbar for larger screen sizes.
The idea behind this is to avoid doing things that other modules could do better than the admin_toolbar_search module.
Result of the proposed changes :
Before:
After:
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | scrollbars-ratio-below-2-to-1.gif | 81.53 KB | ressa |
| #3 | 3558389-3-admin_toolbar_search-mobile-tab-before1a.jpg | 27.67 KB | dydave |
| #3 | 3558389-3-admin_toolbar_search-mobile-tab-after1a.jpg | 43.75 KB | dydave |
Issue fork admin_toolbar-3558389
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:
changes
Comments
Comment #3
dydave commentedQuick follow-up on this issue:
All the changes detailed in the issue summary have been implemented and described in the merge request MR !190 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.
Result of these changes :
Before:
After:
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!
Comment #4
ressaThanks @dydave, great work -- this works much better! The search input field is now shown without any overlap, and disappears when another element is clicked, such as "Manage".
I tried with both "Admin Toolbar Search settings" > "Display the search input as a menu item." enabled and not in Mobile and Desktop, and the search field is shown correctly, without overlap:
"Display the search input as a menu item" enabled
After clicking "Search", the search input field is shown:
"Display the search input as a menu item" disabled
So this looks ready to be committed!
PS. Scrollbars shown below 2 to 1 ratio
I stumbled over an oddity, while testing: It looks like scrollbars are shown if the width / height ratio in Mobile display dips below 2:1. I wonder if it should be fixed here, or in Claro, or somewhere else?
Comment #6
dydave commentedOnce again, thank you so much @ressa for the very helpful and constructive testing feedback! 🙏
Thanks to your detailed description and animation above at #4, I was able to reproduce the issue locally, with a scrollbar displaying indeed 😅
So I tried looking at how other tabs were structured with the core toolbar HTML and CSS classes which allowed me to find the proper way of adding some "spacing" around the search field form element: label + input
Changed
'margin'to'padding'in a few CSS rules to prevent a scrollbar from appearing, which freed up a little bit more space on the right.After doing a fair amount of testing locally and given your positive feedback as well, I thought we should go ahead with these changes and merged the MR above at #5 🥳
Feel free to give the latest 3.x dev a round of testing again and see if the vertical scrollbar still appears 🤞😅
In any case, probably nothing major to worry about and that we should not be able to fix in a new ticket 👌
Otherwise, we could certainly add quick CSS fixes in the next MRs to be committed, such as the ones with Automated Tests. 👍
But most importantly, some of the changes in this ticket were blocking the initial refactoring of the Admin Toolbar Search JS code, which can now become the next and final step of this release. 🥳
I should be able to follow-up shortly with the refactored
admin_toolbar_search.jsfile and will update the Roadmap plan accordingly.But in the meantime, let me know @ressa if you spot anything while testing the most recent changes, in particular with smaller screens, or anything related with the search toolbar, it would be very helpful! 🙏
Marking issue as Fixed, for now.
Just a few more tickets before wrapping up this release.😅
Feel free to let us know if you have any questions, suggestions or comments on this specific ticket or any of the recent changes in the module, we would greatly appreciate your feedback. 😊
Thanks again so much for your help @ressa!
Comment #8
ressaThank you @dydave, for improving Admin Toolbar again and again, it's really fantastic! I feel privileged to help (where I can) with moving all your great work along, and get it included in the Admin Toolbar module.
Thanks for taking a look a the bonus task, of the extra scroll bars :)
I tried the latest dev-release, and your solution to fixing the scrollbars of changing the spacing around the search field form elements from margin to padding works like a charm, and scrollbars no longer pop up under smaller resolutions. And yes, let's keep an eye out for future scrollbars, and deal with them in a follow-up issue.
It sounds great that fixing this issue unblocks further JS refactoring, and I will try to help where I can, and allow the next release. Thank you so much @dydave!
Comment #9
dydave commented@ressa!!! Thanks a lot for the great feedback and all the encouragements, as always! 🙏
Sorry for the delay of this reply, but it took a bit more time than expected to properly fix the ticket for the refactoring of the JS 😅
I realized my ESLINT was broken in VSCode, maybe after updating core to 11.3.x-dev manually.... probably without reinstalling the correct NPM dependencies 🤔
So I thought I might as well install a clean new stack with D11.3.0 and discovered I had a lot more ESLINT errors than expected 😅😆
In particular, all the
@jsdoctags, in doc comment blocks for all JS functions, with param, type, return, etc...It's super cool, helpful and useful... but that added quite a bit more work than I had initially expected 😅
I then struggled a bit with a broken PHPUNIT FunctionalJavascript test, but was able to find the corresponding change. 👌
After a bit of documentation, I was able to create the issue with a merge request: 🥳
#3564229: Admin Toolbar Search: Refactor admin_toolbar_search.js
Feel free to give it a round of tests, I think you will be pleasantly surprised to see quite a few improvements. 🤞
In any case, please let me know if you spot anything or would have any suggestions or ideas, your feedback has already allowed me to fix numerous issues I had not noticed while testing 😅
Seems like we're nearing the next stable release!
Thanks again for your great help @ressa! 😊
Comment #10
ressaYou're welcome, and thank you as well! And no problem at all @dydave, I am just glad I can assist with getting all your great code improvements included in the module!
It was good that you caught the misbehaving ESLINT tool, it also seems to me like npm can be a bit ... unreliable occasionally. So yeah, a fresh install is always a great idea, regularly. I should do that more often, as well 😅
And thank you so much for creating that follow up issue refactoring the JavaScript, and adding extra improvements, like showing the page title, instead of the URL -- a great little improvement. I'll take a look at the updates ASAP, it sounds fantastic that we're getting closer to the next stable release!