Thanks a lot for the detailed description of the issue, with a screenshot, and versions, it's greatly appreciated! 🙏
Indeed, we are aware of the issues currently with the compatibility of the module with the most recent versions of the Gin Theme, see:
✨
Gin theme: Improve compatibility support
Active
I should be able to look into this problem this coming weekend and will hopefully come up with an initial patch to be tested.
In the meantime, please let us know if you spot anything else or would have any questions on the most recent changes to the module, we would surely try answering as soon as possible.
Thanks in advance!
Quick follow-up on this issue:
All the changes detailed in the issue summary have been implemented and described in the merge request MR !205 above at #2.
See the resulting reports after applying the changes:
Fix admin_toolbar.settings:
Fix admin_toolbar_tools.settings:
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.
These changes require Drupal core 10.3 or higher and should probably be merged after related:
📌
Drop support for Drupal 10.2 and below
Active
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!
Quick follow-up on this issue:
All the changes detailed in the issue summary have been implemented and described in the merge request MR !204 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.
These changes require Drupal core 10.2 or higher and should probably be merged after related:
📌
Drop support for Drupal 10.2 and below
Active
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!
Quick follow-up on this issue:
All the changes detailed in the issue summary have been implemented and described in the merge request MR !203 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.
These changes require Drupal core 10.2 or higher and should probably be merged after related:
📌
Drop support for Drupal 10.2 and below
Active
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!
Quick follow-up on this issue:
Created initial merge request !202 above at #2 which should complete the first step towards dropping support for D10.2 and below:
It contains the bare minimum, with the changes to the core version requirements and the removal of all the version_compare code for backward compatibility.
But also a few additional changes:
The Admin Toolbar Links Access Filter module can't be just removed like that from the code base....
Because users upgrading from versions lower than 10.3 might still have the module enabled.
Therefore if the module is removed from the code base it will most likely create issues for uninstalling the module.
The Admin Toolbar Links Access Filter module will have to be removed after the Admin Toolbar module has dropped support for versions lower than 10.3.
So the module will still need to be kept in the next 3.7 minor release and should be removed in the following one (maybe 3.8?!), which should probably have some kind of version constraint preventing a direct update from versions lower than 3.7 (?!).
This should prevent from having issues with sites running db updates (upgrading), unable to uninstall the module since it would have been removed from the code base.
But for the time being: All the code of the module file was removed.
Renamed the admin_toolbar_tools module library toolbar.icon to admin_toolbar_tools.
Moved constants class AdminToolbarToolsConstants back under the tests folder since it now seems to be supported.
Added missing return types to functions where possible.
Refactored CSS file admin_toolbar.sticky_behavior.css to use cascading style.
Since all the jobs and tests are still passing 🟢, moving issue to Needs review for now, as an attempt to get more testing feedback and code reviews.
Feel free to let us know if you have any questions or concerns on any of the suggested code changes or this issue in general, we would surely be happy to help.
Thanks in advance! 😊
Quick follow-up on this issue:
Created initial merge request !201 above at #2 which should cover all the routes and links currently added by the Automatic Updates module:
Added 3 new 'Update' menu links, based on the three local tasks provided by the Automatic Updates module, see:
https://git.drupalcode.org/project/automatic_updates/-/blob/4.x/automati...
1 - Modules update route, under 'Extend > Update'.
2 - Themes update route, under 'Appearance > Update'.
3 - General update status route, under 'Reports > Available updates > Update'.
Added new Functional Tests class to check for all the added links and updated the Gitlab CI configuration to require the Automatic Updates module in its composer job, so the module could be installed for the Tests.
Since all the jobs and tests are still passing 🟢, moving issue to Needs review for now, as an attempt to get more testing feedback and code reviews.
Feel free to let us know if you have any questions or concerns on any of the suggested code changes or this issue in general, we would surely be happy to help.
Thanks in advance! 😊
This issue is starting to run a bit behind:
Could you please try upgrading to the latest admin_toolbar-3.6.3 → release and let us know if the problem still occurs?
It's been almost 6 months since this issue was created and no one else seems to have encountered the same or a similar problem.
Were you able to fix the issue or find what could have caused it?
Feel free to let us know if you have any questions on this comment or this issue in general, we would certainly be very happy help.
Thanks in advance!
The Admin Toolbar Links Access Filter module will have to be removed after the Admin Toolbar module has dropped support for versions lower than 10.3.
So the module will still need to be kept in the next 3.7 minor release and should be removed in the following one (maybe 3.8?!), which should probably have some kind of version constraint preventing a direct update from versions lower than 3.7.
This should prevent from having issues with sites running db updates (upgrading), unable to uninstall the module since it would have been removed from the code base.
Everything should be clear with this issue at this point, thus marking as Fixed , for now.
Feel free to let us know if you have more questions or concerns on any aspects of this comment or this issue in general, we would surely glad to help.
Thanks in advance!
Quick follow-up on this issue:
Created initial merge request !200 above at #4 including the changes to the libraries detailed at #3.
Not sure whether the changes to the JS files could potentially fix another issue
🐛
Disabled, show on scroll-up does not work in Git-version
Active
...
I picked these up while reviewing the various library dependencies and calls in the JS files.
Moving issue to Needs review, as an attempt to get more testing feedback and code reviews.
Feel free to let us know if you have any questions or concerns on any of the suggested code changes or this issue in general, we would surely be happy to help.
Thanks in advance! 😊
Thanks a lot Steven (@steven jones) for suggesting this new feature and providing some details on its implementation! 🙏
Please allow me to try taking a different perspective at the stated requirements:
I'm often logged into production sites and development sites. And sometimes I go to flush caches using the admin toolbar links on my development site...only I accidentally use the wrong browser tab, and flush caches on a production site.
OK, so what we usually do for that is:
- Use
Config Split →
with splits per environment (local, dev, stage, preprod, prod, etc...).
- Use
Environment Indicator →
installed with configured colors, labels, etc... for all splits except prod (disabled). So for example on the dev site the toolbar will be green, on stage blue, etc...
- Disable the admin_toolbar_tools module on the prod site (/for the prod split):
If caches need to be cleared, it can be done on the core system performance page (admin/config/development/performance) and/or by enabling manually the admin_toolbar_tools module, flushing the cache, then disabling it again, or of course, with drush commands.
This type of setup is very standard (I "think"), since these modules are all meant to work very well together:
config_split, toolbar, admin_toolbar, admin_toolbar_tools and environment_indicator
Overall, everything can be done with configuration through the backend without requiring any custom code.
Additionally, the Config Split module is very common to any project with multiple environments, so you might already have it on your site.
We would rather recommend this type of setup, if you would like to ensure the type of issues described in the IS should not even occur.
On some production sites this can cause downtime or at the very least a lot of slowdown and pain for users.
All the more reasons to:
- Prevent users from being able to perform a flushing operation "easily" on the prod site:
==> Disable the admin_toolbar_tools module for the prod split (provided by config_split).
- Make it obvious the environment is *not* PROD, by highlighting the admin toolbar with a flashy color and a clear label of the name of the environment (provided by environment_indicator).
Would you accept some work to develop the following feature:
A way to toggle on/off a confirmation form on the cache flushing routes provided by this module.
Sure! Why not?! 😅
OK for a config switch on/off for this feature, but I would recommend doing everything with JS:
Adding a small JS behavior which would attach a click event listener to the flush links to pop a JS confirm() dialog, just to block the action on the client side.
Very little changes would be needed to implement such a feature, so development should not be too much of a problem.
Let's see if this feature gets a bit more attention or traction and potentially consider whether it should be integrated to project's code base or should sit somewhere else (in its own project repo, for example).
However, once again, I am really not convinced this would be the correct way to approach the initial problem.
I would personally strongly recommend looking at more radical solutions, completely preventing a certain action from being performed on a specific environment.
Once again, thank you very much for your interest in the module and suggesting this feature.
I hope I was to provide a few elements of answer and would be glad to have your feedback on the recommended approach.
Feel free to let us know if you have questions or concerns on any aspects of this comment or the project in general, we would surely be glad to help.
Thanks in advance!
OK Charles (@charles belov), no problem 👌
I've given a go with an AI engine which generated 90% of the feature in a new module, leaving only the review, testing, debugging and final touches.
Overall, it greatly helped speeding up the development of the changes in merge request !199 👍
I've pushed an implementation suggested above with basically two hooks in a module file:
https://git.drupalcode.org/project/admin_toolbar/-/blob/18758a4273873152...
Since this is all very experimental, the feature was added as an extra module for now, which would need to be enabled as detailed here:
https://git.drupalcode.org/project/admin_toolbar/-/blob/18758a4273873152...
Through the backend admin 'Extend' page (admin/modules), should work just fine.
Once the module admin_toolbar_hoverintent_timeout_per_user is enabled, you should be able to start using the feature:
1 - Browse to Edit your user account and a new field should be visible in the form: Admin Toolbar Hover Intent Timeout.
2 - Browse to Edit any user account and a new field should be visible in the form: Admin Toolbar Hover Intent Timeout.
The feature implemented in this new module should cover all the initial requirements from the issue summary.
We would greatly appreciate if you could please try testing the changes from MR !199 and let us know if everything works as expected.
Thus, moving issue to Needs review, as an attempt to get more testing feedback, comments and reviews.
We'll see if this feature gets a bit more attention or traction and potentially consider whether it should be integrated to project's code base or should sit somewhere else (in its own project repo, for example).
Feel free to let us know if you encounter any issues while testing or would have any questions, we would surely be glad to help.
Thanks in advance!
Super nice feedback and help on this issue Charles (@charles belov) and @ressa, once again! 🤩
Great job! 🥳
Thanks a lot! 🙏
✅ OK
I have reviewed all the changes suggested at #5 and the ones in the Issue Summary and rolled them in merge request !198 👍
See additional commit:
https://git.drupalcode.org/issue/admin_toolbar-3564708/-/commit/fb24122d...
I "think", all the changes mentioned in this issue should now be included in this merge request, but it would be great if you could both maybe take another look... thus, moving issue to Needs review 🤞
Charles (@charles belov), could you please take another look at the forms and double check if we could have missed anything?
@ressa, if you could maybe try grepping and such, to see if we could have missed anything else, it would be super helpful as well. 😊
I don't think it would be necessary to change 'hoverIntent' in code comments. 👌
Feel free to add changes and edit directly any of the files from the merge request.
Any comments, reviews or testing feedback would be greatly appreciated. 🙏
Thanks in advance!
Thanks everyone for the great help and feedback!
After getting your positive confirmation and doing a quick round of testing manually, I went ahead and merged the changes above at #7.
All tests and jobs on the 3.x branch still seem to be passing 🟢
This should hopefully get released with the next patch release for the 3.6.x branch.
Marking issue as Fixed, for now.
Thanks again!
Thanks @tjtj for reporting this issue!
Could you please try testing with a different theme and see if the problem still occurs?
Could this issue be caused by a conflict module?
We'd like to get more information on the context, if possible, so the issue could be reproduced.
Thanks in advance!
raaaaaa ....
Good catch Adrian (@styx1983)!!! 😅
Not the first time we've got this issue .... I need to pay more attention to this in the future 😅
🐛
Import of strings skipped due to malformed HTML tag
Active
When I saw this issue in the tracker, I thought it was created by @ressa 😅
who caught this type of issue for us several times before....
Thanks a lot for the MR, I'll take a closer look a bit later... but it's most likely going to get merged as-is 👌
Adrian (@styx1983), did you check if there were any other problematic strings?
Or is it just these ones?!
Thanks again very much for your help raising this issue and contributing a patch! 🙏
The merge request in this issue is now outdated.... given all the changes to the admin_toolbar_search.js in the last 3.6.3 release.
I've started working on the updated MR, but need a bit more time to refine user interactions in the JS....
I'm probably going to have to ask for a bit of help and review from my colleagues more specialized in JS and CSS (Front-end developers), so it's going to take a bit more time ...
Moving back to Active and for the dev branch.... coming back soon to this issue 👌
In any case, any feedback, suggestions, opinions or recommendations for this feature would be greatly appreciated.
Thanks in advance!
Quick follow-up on this issue:
I've done an initial big round of core API updates resulting from the change of module's core version requirements. 👍
I've pushed a bit everything in the same merge request, due to a lack of time, but it will most likely need to be broken down into 4 or 5 different issues/merge requests. 👌
... This is going to be a big change, since we are looking at dropping support for D9...
But we've got to be able to move forward and modernize module's code base with the most recent versions of Drupal and PHP.
We'll probably come back to this issue after fixing a few other easier/smaller ones.
As always, any feedback, reviews, recommendations or suggestions would be greatly appreciated.
Thanks in advance! 😊
@ressa!!! 🤩
Thank you so much for everything and all your great help without which honestly I don't think we would have been able to go this far! 🙏
Sorry for the late reply, but I've already started looking at the next issues, in particular, I've done an initial big round of core API updates to
📌
Drop support for Drupal 10.2 and below
Active
. 👍
I've pushed a bit everything in the same merge request, but it will most likely need to be broken down into 4 or 5 different issues. 👌
... This is going to be a big change, since we would be dropping support for D9... But come on .... We've got to be able to move forward and modernize module's code base with the most recent versions of Drupal and PHP.
I've seen you have also started adding new functional issues, for features improvements, which is great! Thanks a lot! 😊
I've also started looking into
✨
Make obvious when toolbar search returning no results
Active
and should be able to create a merge request within the next few weeks.
We've got lots of interesting issues and topics, whether looking ahead at the compatibility with Gin / Navigation, or the Compact display you have suggested 👍
I'll see if in the midst of all that we could squeeze in a bit of documentation with the
📌
Update the project page
Active
, to take a break from more technical tasks and coding 😊
So far.... Overall, after a week and probably more than 10K installs/updates.... We've got 0 bug raised! 🥳
For a big refactoring release, with 21 commits and 6 months work ... that's pretty amazing!! 😅😆
Let's hope it stays this way 😅🤞
oh ... and of course, I made all the changes you recommended above at #35 to the release notes, last Friday or Saturday 👌
But, please let me know if I missed anything, I would be glad to make more changes if needed.
Otherwise.... Moving ON to 3.7!!! 🥳
Thanks again for everything @ressa!! 🙏🤩
Thanks a lot Michael (@justcaldwell), once again for creating this issue with a very clear description and for looking into how Drupal core seems to address this currently. 🙏
Let's move this to the top of the stack and try to include this in the next release.
I looked a bit at module's code and it looks like we would need to add the following dependencies:
- core/jquery
- core/drupal
- core/once
For libraries: admin_toolbar/toolbar.tree and admin_toolbar_search/admin_toolbar_search.
I think I've picked up a few other changes and would like to add some inline comments here and there in YAML files, but I should be able to create an initial merge request to be reviewed in the coming days.
I don't really see any blocker or major issue that would prevent us from making this change.
Feel free to let us know if you would see anything else that would need to be addressed in this issue, I would be glad to group the changes at the same time in the merge request.
Coming back to this issue very soon....
Thanks in advance! 😊
Super nice James (@jamesoakley), thanks a lot!
Exactly what I wanted to know 😊
Going back to my stacks of tickets 😅
Feel free to ping me directly at any point if there is anything I could do to help.
Thanks again everyone!
Hi Bohdan (@bohart),
I've seen you've been looking after this module for the past years, thanks a lot! 🙏
Sorry to bother you with this, but I wanted to get your opinion on this issue before doing anything, since you have been the active maintainer.
Would you potentially see anything blocking a new stable release for this project's 2.3.x branch?
Otherwise, if that would be fine with you, maybe we could help creating a new release?
It seems I should still have the permissions to add new releases for this project, if that could help.
Any advice, suggestions or recommendations would be greatly appreciated.
Thanks in advance!
Thank you very much Vakul (@vakulrai) for reporting this issue and contributing a patch, it's greatly appreciated! 🙏
The Admin Toolbar Search module's Javascript has been completely refactored in the latest release and this issue should have been Fixed as part of the latest changes.
Could you please try testing the latest release 3.6.3 and let us know if the problem still persists?
This issue was initially reported for the 3.4.2 release, so it is a bit outdated now....
Marking as Closed (outdated), for now.
Thanks again very much for your help and interest in the Admin Toolbar module! 😊
Re #31:
Maybe you already know it, but if not, I heard about https://drupal-mrn.dev/ the other day, so just in case, I'll mention it.
Thanks a lot @ressa! I didn't know this site 🤩
Looks like people have got super lazy 😅
From the same author, I've been using:
https://github.com/mglaman/drupalorg-cli
Which was suggested by Adrian to Jakob in one of the maintainers tickets of this module 😅
I've given a quick test to the tool on the site and it seems to work really well, as well.
The generated markup even seems to include more things than the one from the cli command 👍
I've copied the compare versions link, which is very practical and not included in the HTML generated with the command.
Thanks again very much! 🙏
That's it @ressa! 🥳
The new stable release admin_toolbar-3.6.3 → was created and appears to be available on the project page as well. 👌
Thanks a lot for your great work and continued help to get all these tickets included in this release, once again! 🤩
I "think" there are 21 issues listed in the "Done" section, so we probably fixed between 25 to 30 issues, counting the ones for support requests, for example. 🏆💪
Overall, a lot of refactoring work, code cleanup and review have been done, which is a big step towards module standardization. 👍
Module's automated tests coverage was greatly increased and the code base was brought into compliance with all the validation jobs available on Gitlab CI: passing all 🟢
Things are starting to feel like they're held a bit tighter in the code base.
However, we might very well have broken some things here or there, which we could not necessarily have tested with our setups 😅
and thus, we probably need to be prepared for incoming bug reports/issues, as usual 🙈😅
No problem at all for creating quick follow-up patch releases if needed, as we did before.
Otherwise, I should probably start drafting a 3.7 Roadmap plan, which would mostly include dropping support for D<10.3 and should allow us to start modernizing module's code base with the latest standards and APIs. 🤓
Of course, we should also probably re-evaluate the max bundle number feature with issues such as
📌
Spell it out which sub-menus are affected by Maximum number to display setting
Active
... and many other tickets in the queue: Support for core Navigation, Gin Theme, Accessibility, etc...
Let me know if you spot anything in the release notes or maybe something I could have missed, I would be glad to make adjustments to the documentation. 😊
Thank you very much @ressa for all your contributions, constructive help and sincere involvement in the Drupal and Open Source community!
Here we are @ressa!!! 🥳
Everything is in the box now 🤩
I couldn't stop myself from adding a few more changes in this release 😅
With the ESLINT fixes and the one to add declare(strict_types=1) to PHP files...
I've just added them to this issue as well, for documentation purpose.
Just need to tag the release and create the release notes .... and we'll be done with this round 👌
New stable release coming up shortly! 🥳
Quick follow-up:
Since all the tests and jobs were still passing 🟢, including the D9 pipeline, I went ahead and merged the changes above at #8 🥳
This should allow us to intercept changes to core modules or APIs, much easier, with the different core versions being tested in pipelines, so we might be able to react quicker and fix any potential compatibility or support issues with newer core versions.
We can certainly revisit these test classes in the future, to maybe move some of them to Kernel tests, but for the time being, we could probably consider this issue as Fixed, for now.
Feel free to let us know if you have any questions or concerns on any aspects of this ticket or the project in general, we would surely try answering as soon as possible.
Thanks again everyone for the great help and feedback on this issue! 🙏
I can see a bit better what Joachim (@joachim) meant with using Kernel tests instead of Functional....
In particular, I've had a small sample of the difficulty of getting these tests to stay compatible with multiple core versions:
I had to add special cases for supporting tests for D9 and revert the change suggested above moving the constants to the tests folder ...
It seemed to work with D10 and D11, but not D9 😖
See the two last commits in the merge request.
Overall, the great complexity of these tests mostly stems from the "incomplete" or "unbalanced" implementation of the max bundle number feature...
Otherwise, these tests should be much more complete than the existing ones and should provide a much better coverage of module's integration with other core and contrib modules.
Therefore, let's move forward with what we have in this MR and we'll see how to make this base of tests evolve in the future.
Pending merge ....
dydave → changed the visibility of the branch 3550604-automated-tests-add-debug-d9 to hidden.
Quick follow-up on this task:
Overall the Admin Toolbar Search module FunctionalJavascript tests were completely refactored to be mostly re-focused on testing the JS code of the module: admin_toolbar_search.js.
Basically, the tests install the required modules, then perform several searches and ensure the expected results are found in the HTML of the suggestions updated by the JS code.
All the current logic of the integration tests with 'admin_toolbar_tools' was moved out of the 'admin_toolbar_search' module to Functional tests in the 'admin_toolbar_tools' module, in related issue
📌
Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks
Needs review
with merge request:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/173
In particular, any logic related with SearchLinks and loading of extra links in the search autocomplete was moved to the integration Functional test class AdminToolbarToolsExtraLinksTest, along with any creation of entity bundles, etc...
See also trait AdminToolbarToolsEntityCreationTrait in related issue's merge request.
Fixed tests for query strings striped in search for issue 🐛 Exclude random token string in link from Admin Toolbar Search Active :
To test the issue locally: restore the query string in the search with the following JS test code:
// Strip query strings from link URLs for searching to prevent generated
// tokens or destinations from appearing in the search results.
// const linkUrl = element.linkUrl.split('?')[0].toLowerCase();
const linkUrl = element.linkUrl.toLowerCase();
and run the test to confirm it fails the check on assertSuggestionsNotContain in class AdminToolbarToolsSearchTest.
Since all the tests and jobs still seemed to be passing 🟢, I went ahead and merged the changes above at #5.
Only tests files were changed in this issue so it should not have any impact on the module.
We'll see how to make these tests evolve in the future, as more changes are made to project's code.
Marking issue as Fixed, for now. 🥳
Feel free to let us know if you have any questions or concerns on any of the latest code changes or the project in general, we would be glad to help.
Thanks in advance!
Quick follow-up on this task:
Added: declare(strict_types=1); to all PHP files except for '.install' or '.post_update.php'.
Overall, I don't think I missed any file, but if I did, we can still add the strict types declarations as we make more changes to PHP files in the future.
Since all the jobs and tests seemed to still be passing 🟢, I went ahead and merged the changes above at #3. 🥳
Marking issue as Fixed, for now.
Feel free to let us know if you have any questions or concerns on any of the latest code changes or this project in general, we would surely be glad to help.
Thanks in advance!
Thanks again @ressa for the great help documenting and describing this issue, as usual! 🙏
Funny thing, but the solution described above at #2 is not all the one that was implemented in the end 😅
The reason the solution above was not satisfying is because we could not just save menu links URLs without the query string, since it would be lost when building search results suggestions when the real links would need to be displayed, for example, the logout link, etc...
Therefore, the query string needs to be striped off when the search is performed at run-time (in function handleAutocomplete ).
Quick update on this issue following the latest code changes:
This issue should have been fixed in related issue
#3564229: Admin Toolbar Search: Refactor admin_toolbar_search.js →
, in particular, with commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/40fc5b84a8aad7...
with the following piece of code:
https://git.drupalcode.org/project/admin_toolbar/-/blob/40fc5b84a8aad768...
// Strip query strings from link URLs for searching to prevent generated
// tokens or destinations from appearing in the search results.
const linkUrl = element.linkUrl.split('?')[0].toLowerCase();
Additionally, the automated tests related to this feature have been added to related:
📌
Automated tests: Simplify admin_toolbar_search FunctionalJavascript tests
Active
, in particular, with commit:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/175/di...
Since I remember you already tested this and confirmed it worked as expected, in the JS refactoring issue and that some tests coverage should be put in place for this piece of logic, we should probably be able to consider this issue as Fixed, for now.
This comment should document all the work related with this issue and allow us to circle back on the corresponding code changes if needed in the future.
Feel free to let us know if you spot anything else that we could have missed in this issue, or create a new one at any time, we would certainly be glad to take another look. 👍
Thanks again for all your great help @ressa!
Great catch, once again, really not an easy one! 🥳👍
Referencing related issue:
#3564883: AT Search: Improve tests and documentation for admin_toolbar_search.js →
Hey @ressa! 😊
Is this issue still occurring on your setup?
I've been testing with the latest stable 11.3.0 and the 11.x dev branch checkout out from the repo, but I can't seem to be able to reproduce the issue.
Could you please maybe try on a fresh setup and see if the problem persists?
In which case it would be great to collect additional context information so we could try reproducing the issue.
Otherwise, from the latest rounds of tests I've carried locally, the sticky behaviors all seemed to be working fine (as expected).
Feel free to let me know if the problem is still occurring on your side, I would surely be glad to take a closer look. 👌
Thanks in advance!
Thanks again everyone on this issue!
Overall, it was too complicated to get all these changes reviewed, tested and merged at the same time, so the two JS files were broken down in two separate issues:
- admin_toolbar_search.js: #3564229: Admin Toolbar Search: Refactor admin_toolbar_search.js →
- admin_toolbar.js: 📌 ESLINT: Fix admin_toolbar.js validation errors Active
The changes have been merged and the ESLINT job is finally passing 🟢 \o/
Marking this issue as Fixed, for now. 🥳
Thanks again very much for the great help keeping the module well maintained! 🙏
I have been working for some time on rewriting the file 'admin_toolbar.js' with Vanilla JS and was able to make significant progress.
But it's probably less risky for now, before creating a new stable release, to just fix the ESLINT errors and keep the existing logic with jQuery.
Mostly the file seems to do two things:
1 - Remove the 'title' attributes from menu links to prevent conflicts with the menu dropdown, introduced in issue:
#2630724: Consider changes to link title parameter to avoid tooltip visual conflict →
The way this was implemented could probably be improved, with a bit less radical method, avoiding to removing the title attribute completely.
We could maybe try wrapping links inner text with a span tag, with an empty title attribute:
<span title="">
which would allow keeping title attributes and just prevent the display of tooltips.
But this would most likely need a specific ticket, to be discussed in more details.
2 - Provide a very basic keyboard navigation, introduced in:
#2873228: Toolbar menu accessible and navigable with keyboard →
The existing logic with jQuery was kept unchanged for now.
Refactoring with Vanilla JS should probably also be addressed in a separate issue.
Otherwise: removed the two last blocks of jQuery code at:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/js/admin_t...
$('.toolbar-menu:first-child > .menu-item', context).on('hover', function () {
$(this, 'a').css("background: #fff;");
});
$('ul:not(.toolbar-menu)', context).on({
mousemove: function () {
$('li.menu-item--expanded').removeClass('hover-intent');
},
hover: function () {
$('li.menu-item--expanded').removeClass('hover-intent');
}
});
Since I was unable to really test these pieces of code.
Added changes to the file 'admin_toolbar_search.js', based on the feature discussed at
#3564229-7: Admin Toolbar Search: Refactor admin_toolbar_search.js →
.
Overall, these changes should be very localized and any potential impacts would only be around the keyboard navigation.
Therefore, since these changes seemed rather safe and all the jobs and tests passed 🟢, I went ahead and merged the changes above at #4. 🥳
The build pipelines are finally coming back green 🟢, without any warnings.
A follow-up task was created to address the points above:
📌
Improve admin_toolbar.js
Active
so we could come back to these changes at a later point.
Marking issue as Fixed, for now.
Feel free to let us know if you encounter any issues with the latest code changes or would have any questions on the module in general, we would surely be glad to help.
Thanks in advance! 😊
Thanks a lot Charles (@charles belov), once again, for suggesting this very interesting feature.
After doing a quick search in the module, I found the following piece of code:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...
// Add the configured hoverIntent settings values to the JS of the toolbar.
$items['administration']['#attached']['drupalSettings']['hoverIntentTimeout'] = $hoverintent_behavior['timeout'];
In other words, the hover delay is currently injected in the Javascript of the page through a 'drupalSettings' variable, which is set in the PHP code (module's file).
After doing a quick search on how this JS variable could be overridden, I found:
- API documentation for function hook_js_settings_alter and
- Documentation on how to override a drupalSettings JS variable
Which means:
By implementing hook_js_settings_alter in a custom module, it should be possible to override the value of the hover delay (hoverIntentTimeout).
The delay value could then be configured in a custom field in user's account at /admin/config/people/accounts/fields, which would be saved in the database and attached to user's information, perhaps as "Preferences".
The value could then be injected in the page with a small piece of PHP code (hook_js_settings_alter), reading the value from current user's account, defaulting to the one defined in the module's configuration.
Overall, this solution would probably require 10 to 20 lines of code in a custom module + some site building/configuration to add the field in user accounts.
Any permissions could be controlled through the core field system and outside of module's scope.
With this type of implementation, it could perhaps be contributed and built as a recipe?! 🤔
@TODO: Recipes should most likely be further investigated for something like this:
Documentation for Drupal Recipes →
I think you would understand the costs of adding a new feature to the module, in terms of tests, maintenance, documentation, support, etc...
Therefore, if a particular feature could already be achieved with a different module, component, API or approach in general, we would rather tend to recommend using the existing tools at our disposal, instead of directly considering making code changes to the module.
Could you please take a look at the recommended potential solution and let us know if it could be going in the right direction?
(Fulfilling the requirements)
Feel free to let us know if you have any questions or concerns on any aspects of this reply, this issue, or the project in general, we would certainly be glad to help.
Thanks in advance!
Thank you very much Charles (@charles belov), as always, for your very constructive and helpful feedback.
No problem and thanks a lot for the suggestions. 👌
✅ OK for the requirements:
Expected result:
a. The label for the check box corresponds to what happens when you check the box.
b. Hover intent is two words
✅ OK for the labels on the Admin Toolbar Tools settings page at /admin/config/user-interface/admin-toolbar-tools:
Enable/Disable local tasks display
Local tasks such as node edit and delete.
🛑 KO But I can't seem to find:
Enable/Disable the hoverintent functionality
Check it if you want to enable the hoverintent feature.
I also searched module's code base and could not find these labels...
Could you please provide more details on where these textual elements could be found?
Otherwise, could you please review the Admin Toolbar settings form at /admin/config/user-interface/admin-toolbar?
In particular, the text hoverIntent appears multiple times.... See screenshot below:
Initially the idea behind using a single word was probably to reference the Javascript library and its corresponding effect.
However, if you have recommendations on replacing any occurrences of the word, with two words in the form, they would surely be greatly appreciated.
We would probably need to look more generally at replacing any occurrences of the word in form labels.
Just to let you know: We've already done a few rounds of improvements on these forms with @ressa and he could definitely be of great help to review and validate textual changes to these forms.
Ideally:
First, we would like to collect as many required changes as possible, so they could all be processed at once and tested exhaustively in a single round: Trying to minimize the amount of back and forths we might need on an issue like that.
Please let us know your suggestions of adjustments to be made to the forms and we will be glad to make the corresponding code changes.
Thanks in advance for your feedback and help!
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
📌
Admin Toolbar Tools: Remove dependency on Admin Toolbar
Needs review
.
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... 🥳
Added possible future improvements, aiming at the very least, at providing Admin Toolbar Search support for the Gin Theme and Core Navigation module.
To be tested more carefully and probably detailed/described in a specific issue.
Added requirements for Admin Toolbar Search, which should probably be further tested, then detailed/described in a specific ticket.
Note: I personally don't use this feature, have never used it and had no idea it actually existed before looking in depth at module's JS file.
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! 🙏
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! 🤞
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! 😊
@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 @jsdoc tags, 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! 😊
Adding the last refactoring issue for the Admin Toolbar Search module 🥳🤞
#3564229: Admin Toolbar Search: Refactor admin_toolbar_search.js →
which should also fix:
🐛
Exclude random token string in link from Admin Toolbar Search
Active
We've got part of the ESLINT completed as well, with the only file left 'admin_toolbar.js'.
Once these changes could be merged, the tests tickets should be merged as well and the new release created! 👍
Updated the target version to 3.6.3, since this release should not be introducing any BC breaking changes.
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.
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:
- Give the ability to hide certain menu items in the breadcrumbs, displayed in search suggestions, when they are assigned the CSS class
'.admin-toolbar-search-ignore', see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
Is that something really useful or necessary?
-
Exclude the Front page from search results, with url
'/', see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
Is that something really useful or necessary?
Is it such a big deal to show the Front page in search results?
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:
- Provide different ways to load the Search field on the page (not only in the toolbar) ==> Support also Navigation, Gin, etc...
See: ✨ Gin theme: Improve compatibility support Active . - Allow the menu links selector to be configurable to pick up links from any menu or markup on the page, for example to support core Navigation and pick up its links instead.
See: #3556898: Admin toolbar tools: Improve support for core Navigation → .
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!
Once 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.js file 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!
Thanks a lot Thomas (@drupatz) for your positive feedback and confirmation the patch fixed the issue! 🙏
Following your confirmation, I added a few minor changes to the doc comments blocks of the new Functional tests file and did another round of testing manually locally.
The links added by the module worked great with the correct order in the admin toolbar menu. 👌
Since everything seemed to work fine and the automated tests or jobs were all passing 🟢, I went ahead and merged the changes above at #28. 🥳
Note the 'conflict' constraint in the composer.json file seems to work fine as well, since composer jobs for certain core versions (previous major, for example) appear to be unable to find a compatible version (greater than 2.1.0) package and thus, required a special case to conditionally skip the Functional test. 👍
The changes should be released soon with the upcoming 3.6.3...
This is definitely an important issue we're glad to cross off our list... just a few more to go 😅
Since I don't see anything else remaining or any follow-up task for this ticket for the moment, it is probably safe to consider it as Fixed for now.
The added Functional tests should allow us to detect any breaking changes with the different versions of the Project Browser module and be able to address them quicker or even anticipate them earlier.
This should make the two modules integration more robust and hopefully, decrease the amount of maintenance work on our side or support requests we get to fix this particular feature 😅🤞
In any case, feel free to let us know if you still encounter any issues with the two modules, or would have any questions on any of the most recent code changes, we would certainly be glad to help! 😊
Once again, thanks a lot to everyone for the great help keeping the Admin Toolbar Tools module well maintained! 🙏
Cheers!
Thanks a lot @mengi and @jungkunwar for the efforts taking an initial stab at module's documentation! 🙏
Documentation related issues are not necessarily the most popular ones 😅
so your help and contributions are even more highly welcome.
This is just a quick reply to let you know the module is still undergoing some major refactoring with the likely evolution of some features, perhaps in the next coming releases, see for example:
🌱
[Meta] Roadplan for Admin Toolbar 3.7
Active
or most importantly:
#3261664-15: Bundle sub-menu maximum not honored →
with the potential removal of the feature and config max_bundle_number.
Since we might not want to work on documenting a feature or setting that could be removed, we recommend holding off on revamping module's documentation a bit longer.
However, we could perhaps split up this issue in sub-tasks for the different sub-modules:
Admin Toolbar Search should be the first ready to be documented, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
We should probably create a documentation guide with pages for each sub-module and could start with the Admin Toolbar Search module very soon.
Same thing as for other modules: Document installation, Settings form screenshots, etc...
In any case, additional comments, reviews or feedback would definitely be greatly appreciated!
Thanks again for the great help with module's documentation! 😊
Updated slightly two regular expressions (regex) so they could also match with the markup on Drupal 9.
Since the changes still seemed to pass all jobs and tests with current Gitlab CI configuration 🟢, they were merged above at #3. 🥳
Created a new build Pipeline Schedule at: Deprecation build: D9.5.x
https://git.drupalcode.org/project/admin_toolbar/-/pipeline_schedules
With variables overrides specifically for testing D9:
OPT_IN_TEST_PREVIOUS_MAJOR: 0
OPT_IN_TEST_PREVIOUS_MINOR: 0
OPT_IN_TEST_NEXT_MINOR: 0
OPT_IN_TEST_NEXT_MAJOR: 0
OPT_IN_TEST_MAX_PHP: 0
_PHPUNIT_CONCURRENT: 0
DRUPAL_CORE: '9.5.x'
Configured to be executed once every month for the main 3.x development branch. 👌
But this also allows triggering the build manually immediately, see an example where the build came back green 🟢 on D9:
https://git.drupalcode.org/project/admin_toolbar/-/pipelines/669173
Therefore, although this configuration is not integrated with the regular build pipeline, for every merge requests and such, it could still provide regular testing of the compatibility of the module with older (deprecated) Drupal core versions.
Additionally, it could serve as a Gitlab configuration template example, when having a doubt on a change for D9, it is still possible to override the variables of a build to specifically test Drupal 9.
If a more robust or automated solution needs to be implemented in the future, we could still consider working on the Gitlab CI configuration file, but for now, this simple and quick solution available from the Gitlab CI back-end should probably be a good start. 😊
Marking issue as Fixed, for now.
Thanks in advance!
Fixed the PHPSTAN issues first by adding an explicit type to the variables.
Fixed also compatibility issues of the 'responsive_image_link_formatter' Kernel tests with Drupal core 11.3.x.
It seems the dependencies need to be explicitly declared in modules tests class.
The changes were merged above at #3 and the dev pipelines are back to passing 🟢 \o/
https://git.drupalcode.org/project/image_link_formatter/-/pipelines/669062
Marking issue as Fixed for now.
Thanks!
Could anyone encountering this issue please try the patch from MR !185?
Download the patch in plain diff format at the following URL:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/185.diff
and apply it to your project as for any patch for contrib modules.
We're waiting on additional feedback and reviews before getting these changes to the module....
Thanks in advance!
Let's keep going @ressa!!! 🤩
I've just added another big one to the list:
📌
Admin Toolbar Search: Refactor mobile toolbar item to use core toolbar tray
Active
I would love to have your opinion and feedback on this one as well: More clean-up, more standardization, better focus on what the admin_toolbar_search module should really do. 👍
Once this one is done, I'll push one more big one with the clean-up and refactoring of the admin_toolbar_search JS and we should be good to WRAP UP this release! 🤞😅🥳
As always, your feedback is more than appreciated!
Thanks in advance!
Quick 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!
Looks like the core navigation is about to switch to stable in 11.3.0, see:
#3421969-99: [PLAN] New Navigation and Top Bar to replace Toolbar Roadmap: Path to Stable →
, in particular:
1. Mark navigation as stable (remove the experimental flag, update d.o docs)
2. Use navigation in the standard and umami profiles.
3. Deprecate toolbar module (and the various sub-issues that will be required to do that).
The ticket seems to mention a Top Bar in its title, but I was unable to find any screenshots or clear issues showing how the new navigation would be integrated in the top bar... 😖
Would anyone have any information on that?
This is more related to admin_toolbar and the JS dropdown of the module, than the extra links added by admin_toolbar_tools, but since we have screenshots here, I thought I might ask. 😅
Thanks in advance!
Thanks @ressa!
Looks like there are going to be some substantial changes to the core Navigation in the 11.3.0 release, see:
#3421969-99: [PLAN] New Navigation and Top Bar to replace Toolbar Roadmap: Path to Stable →
I wonder if this could be somehow connected...
I'll take a closer look when I get a bit more time to test the upcoming 11.3.0. 👌
Thanks in advance! 😊
Thanks @dark05 for the follow-up on this issue and for trying the changes from the MR. 🙏
Indeed, the changes in the merge request were made some time ago and things could have changed in between on the Gin Theme side....
This is going to require investigating closer in the code....
If anyone has time and fancies working on this, contributions are more than welcome! 😊
Otherwise, I'll come back to this issue after the ones on which I have been working for improving tests.👌
Thanks in advance!
Quick follow-up on this issue:
Since the changes required to fix the development pipelines were really small and all the tests and jobs were still passing 🟢, I went ahead and merged the changes directly above at #3, so other merge requests could be rebased and tested properly again 👌
Note: this change is effectively tested in several places in the module, where the class 'toolbar-icon' is checked, for example here:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/tests/src/Fu...
Marking issue Fixed, for now.
Thanks! 😊
Thanks a lot @dark05 once again, for raising this issue! 🙏
We have decided to move compatibility issues related with the Gin Admin Theme → to a specific related issue: ✨ Gin theme: Improve compatibility support Active .
Marking this ticket as Closed (duplicate), in favor of the more generic issue.
Let's move the conversation over there. 👌
Thanks in advance for your comments and feedback! 😊
Thanks a lot Andrew (@sprocketman) for raising this issue! 🙏
We have decided to move compatibility issues related with the Gin Admin Theme → to a specific related issue: ✨ Gin theme: Improve compatibility support Active .
Marking this ticket as Closed (duplicate), in favor of the more generic issue.
Let's move the conversation over there. 👌
Thanks in advance for your comments and feedback! 😊
Thanks you very much Bron (@bron.f) for reporting this issue with a clear description and screenshot, it's very helpful! 🙏
I can confirm I was able to reproduce the problem with the steps described in the issue summary.
I have added the permission 'access taxonomy overview' to the 'Content editor' role, which reproduced the problem in the screenshot.
However, I was unable to reproduce the issue with other entity types, such as 'Block Content' or 'Media'. 😅
Assigning the permission 'administer media types' for example also seems to grant the permission to delete items and they will always have a child link to display: Delete
In short, it seems the menu links for the taxonomy terms wrongly receive the HTML class 'menu-item--expanded', for some reason.
But this should not be specific to the Admin Toolbar module and the behavior could probably be reproduced with other menu related modules using the Drupal core Toolbar, see:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/toolb...
This is where these classes are set, the Admin Toolbar module only adds some CSS styles...
Therefore, we would prefer if this issue could be addressed directly in Drupal core, rather than trying to implement some custom logic in the module to fix a wrong behavior, on which it relies.
Could you please try to see if you are able to reproduce this issue with any entity type other than 'Taxonomy'?
Could you please try to see if there would be any related issues in Drupal core? Maybe related to menu links or the toolbar?
Any additional information or context on the problem would help greatly better identifying a potential solution. 👍
Feel free to let us know if you have any questions or concerns on any aspects of this comment, or the module in general, we would surely try answering as soon as possible.
Thanks in advance!😊
Thanks so much @ressa for your great reviews and reporting back on all your tests!
We have made great progress already, with a big round of "standardization" and clean-up, with the names of the css files, icon images, inline code comments, etc...
Not to mention the improved base of automated tests! 🥳
The two remaining test issues are probably too demanding, and I hope you or someone else can review/finalize them, and maybe also the Project Browser issue?
No problem at all:
The two remaining issues are related with phpunit automated tests and should have no impact on any of the files of the module. 👌
None of the files currently in the merge requests of these issues should be outside of the tests folders.
I still need to make a few changes to these MRs by the way: add a few more checks in the FunctionalJavascript tests...
So I'll definitely take care of these issues myself.
Let's keep the Project Browser issue opened in NR a bit longer to see if we could get any feedback on the latest changes to the merge request. It is actually very easy to test: Install the latest versions of project_browser and admin_toolbar_tools ==> Crash 😅
I've seen a duplicate ticket created recently and I'm afraid this might happen more frequently and require to be released sooner...
Let's see if we could get more testing feedback on the latest suggested patch and we should be able to get this issue fixed as well 🤞
Thanks in advance @ressa for your testing feedback and reviews! 🙏
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
📌
Update the project page
Active
.
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
✨
Bundle sub-menu maximum not honored
Needs work
. 😅
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:
- 📌 Automated tests: Simplify admin_toolbar_search FunctionalJavascript tests Active
- 📌 Automated Tests: Add Functional tests for classes ExtraLinks and SearchLinks Needs review
Which should allow further refactoring of these files. 😅
Marking this task as Fixed, for now.
Thanks again very much for your great help @ressa! 🙏
Thanks Rajab (@rajab natshah)!
Sure, I understand and saw the history on this issue but if possible we would like to keep this merge request opened:
I "think" we might still want to merge these changes, but haven't had time to test or review them properly myself yet.
No problem for creating a new ticket with a new merge request, since this one doesn't apply anymore.
But if possible it would be good to keep this issue opened a little bit longer so these changes are not buried. 😅
We should create a follow-up ticket from this issue, with the same changes, and move this one back to Duplicate. 👌
Any advice, comments or suggestions would be greatly appreciated.
Thanks in advance! 😊
Thanks @ressa! 🥳
I was finally able to update the documentation page and it is now displayed at the top of the "Block Class" guide: 👍
https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →
I also have greater maintainer permissions now, which should save us some time with these types of administrative tasks in the future 😅
Back to Admin Toolbar now, for a bit longer! 👌
We've made great progress, but still have a long way to go. 😅
In particular, all the work that will need to be done with the Documentation as well, see:
📌
Update the project page
Active
I've been thinking of this, looking at the README files recently for the refactoring ticket for Admin Toolbar Search...
This is still going to take some time 😅
Thanks again for all the great help and suggestions @ressa! 🙏
Thanks a lot Alberto (@avpaderno) once again for the speedy reply! 🙏
The change seems to have worked, since I was able to update the menu of the new documentation page so it could be added to the Block Class guide. 👍
Everything should be in order now with module's documentation and I should be able to make any changes in the future 👌
Thanks again for the great help and speedy reply! 😊
Thanks a lot @ressa for taking the time to test and review this issue as well! 🙏
I knew you might be interested to see how the module could work with only the core toolbar module enabled. 😉
Following your confirmation, I rebased the MR and fixed an issue with a failing test missing a dependency on 'admin_toolbar'.
I thought about potentially adding a standalone test class in this MR, but there are already a few refactoring tickets pending, with a lot of moving pieces around admin_toolbar_search tests, so I thought we might want to come back to this a bit later, once other major refactoring changes could have settled and created the follow-up task: 👌
📌
Admin toolbar search: Improve automated tests
Active
Since all the jobs and tests were still passing 🟢, I went ahead and merged the changes above at #5. 🥳
But it works well, by itself -- though it's a bit limited, since only the eight top level menu items are available.
Indeed @ressa, the idea behind this is maybe down the road, to allow the module to expose some kind of hook or plugin manager so it could collect extra links added by other modules, for example.
Or it could maybe search in the links from the core navigation instead of the Toolbar... (?!) 🤔
But this is an initial step towards helping to extend module's features and better integrate with other modules. 🤞
Marking this issue as Fixed, for now.
Still more pending tickets in the queue! 👍
Thanks again very much for your great help @ressa! 🙏
Thanks a lot @ressa for taking the time to test and review this issue!
I knew you might be interested to see how the module could work with the new Navigation module. 😉
Following your confirmation, I thought I might consolidate a bit the changes from the MR, in particular, add a specific Functional tests class for the standalone 'admin_toolbar_tools' module.
So I started adding the test class and realized the checkbox to select show_local_tasks was useless without the Drupal core Toolbar module, so I added a quick check for that. 👌
Once I was satisfied with the initial tests class and that all the jobs and tests were still passing 🟢, I went ahead and merged the changes above at #5. 🥳
I also created a follow-up issue to #3556898: Admin toolbar tools: Improve support for core Navigation → , with the resulting screenshots from this issue at the top, so we could easily pick this up later where we left off. 👌
Marking this issue as Fixed, for now.
Still more pending tickets in the queue! 👍
Thanks again very much for your great help @ressa! 🙏
Thank you very much @epmcdole for creating this issue, with an error message, it's greatly appreciated! 🙏
Could this issue be a duplicate of the following one?
🐛
TypeError Unsupported operand types int + string in Drupal\admin_toolbar_tools
Active
Could you please help us test the patch from MR !185 and report back in the issue?
Marking issue as Closed (duplicate), for now, but feel free to let us know if the suggested patch still does not fix the problem in you case.
Feel free to let us know if you have any questions or concerns on any aspects of this comment, related issue or the project in general, we would surely be glad to help. 👌
Thanks in advance! 😊