🇫🇷France @dydave

Thanks @andypost!

Any chance you could help us create the new branch and clarify what would be needed to get this issue moving?

It is currently still blocking the upgrade for projects.

Any advice, suggestions or feedback would be greatly appreciated.
Thanks in advance!

Thanks a lot @ressa for getting this going! 🙏

Ideally, I would like to get the following issues merged before creating the next release:

• #3487246-16: Warn site owners about the removal of Admin Toolbar Links Access Filter → :
  No impact at all on the module, so very safe. Additional reviews of the wording, text/message would be appreciated.
• 🐛 Some mandatory parameters are missing ("source") to generate a URL for route Active :
  Pretty much ready to go: It has received feedback from several users, but more testing and reviews would certainly be welcome.
• 📌 GitlabCI: Fix ESLINT validation errors Active :
  This is a big one.... The impact is high and should maybe be split into two tickets, one for admin_toolbar.js and one for admin_toolbar_search.js.
  It has already received quite a lot of testing though from @andresalvarez and myself. But more testing is definitely required.

 
Overall, the most sensitive issue is the one related with the work on ESLINT and the impacted major JS files.
The problem is that currently the module doesn't have FunctionalJavascript Tests for admin_toolbar...
It does seem to have FunctionalJavascript Tests for admin_toolbar_search though, see:
https://git.drupalcode.org/project/admin_toolbar/-/tree/3.x/admin_toolba...
But I'm not sure how reliable they could be and the types of issues that may arise with certain browser compatibility issues, etc...

On the side, I've started working in a local branch on converting admin_toolbar.js to full Vanilla JS, so I was hoping I could combine this work with the ESLINT fixes in a ticket, which would only really leave testing the admin_toolbar_search.js (JQuery).

But if we find it too risky for this release and hard to test properly, we could also consider moving the ESLINT fixes to the next release and tag a new stable straight away 👌
 

Otherwise if we receive positive feedback and reviews on these issues and the corresponding merge requests, we should definitely be able to tag the new 3.6.0 stable release.
Thanks in advance! 🙂

Fantastic news @ressa! 🥳

Thanks a lot for testing carefully the upgrade process from stable 👍

Following your confirmation, I gave the MR a quick round of manual testing locally and a final review.
Since everything looked good 🟢, I went ahead and merged the changes above at #30 🥳

Another nice feature has landed... after doing a bit of exploration and getting a better understanding of the overall hoverIntent features 😅

Feel free to create new tickets if you encounter any issues with any of the latest CSS/JS changes and the newly added features.
If you have more ideas, like this one or the slide-on-scroll one: please share them with us 🙂

Marking this issue as Fixed, for now.

Let's keep moving forward with the next issues on the 📌 [Meta] Roadplan for Admin Toolbar 3.6 Active 👌

Thanks again to @aaron.ferris, @redeight and @ressa for the great help! 🙏

Thanks @ressa!

Looks like we're making a good team! 🙏
Really glad we found each other on this module 😅

Thanks a lot for the META ticket!

I'll get it updated as we go, definitely! 🥳

Right right right !!! @ressa, you need to run DB updates!! 😅

Thanks a lot BTW for the very nice feedback, once again! 🙏

Indeed:
We can't move from DEV to this MR like that ... because the update hook was changed between the previous change in 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active and this issue....

In other words, the update hook in the DEV (3.x) is not correct anymore for this merge request.

To avoid having the warning, you would have to download the DEV version, patch it, then install it.

Otherwise, to fully test the update hook from this MR, you would have to go back to 3.5.3 (stable), install the module (drush en admin_toolbar), then upgrade the module to DEV (3.x), patch it, then run drush updb (DB updates) to make sure the correct config objects are properly created.

Let me know if that helps.
As far as I can see, this shouldn't be a problem with the code, since the update hooks that were added in other tickets can still be changed since they have not yet been released as stable (still considered as DEV).
Back to Needs review.

Looking forward to your feedback!
Thanks in advance !

Great job @mparker17 and @spiderman! 🥳

Thanks a lot for the great documentation in this ticket and the very clean approach taken in the merge request to deprecate previous API signatures. It's definitely a great example and source of inspiration to me in terms of steps you have taken to drop support for certain versions 👍

Glad to see the project moving forward!
Thanks again for all the great work to keep the module well maintained! 🙏

No activity on issue for more than a year.

Could you please try updating to the latest stable version and test if the problem still occurs?
Maybe this issue was fixed in other interfering modules or themes?

Closing issue as Outdated, for now.

Thanks everyone for all the work!

OK, back to the original version, clean and simple, with:

• Single open fieldset
• Single checkbox enable/disable
• Single config field with select dropdown: Timeout / displayed only when hoverIntent enabled

 
Created the new merge request MR!137 above at #23 so the other more complete version could be kept on the side for reference.
 
Back to the previous, simple version, which I personally find much better and easier to understand 😅
Additionally, this is probably a big enough step for this feature at this point. We could certainly come back to the changes from the previous MR with the interval and sensitivity settings at a later stage, if any interest is shown for this feature 👌

Since all the jobs and tests still seem to be passing 🟢, moving issue back to Needs review as an attempt to collect more reviews and testing feedback.

Thanks very much in advance! 🙂

dydave → changed the visibility of the branch 3440852-make-un-hover-delay to hidden.

Agreed! Thanks @ressa!

I think I got carried away above at #17 and had not realized the sensitivity and interval settings would not be very useful or as difficult to use or understand as they would seem to be 😅

After all, I thought about it again and came down to: if we are not really going to use these settings, we should not be bearing the weight of their maintenance, support and testing.

I'm going to create a new merge request through, since I'd like to keep the changes from MR!99 in the repo somewhere, even if the stale feature branch gets deleted in the future. There are a lot of labels/descriptions, schema, settings definitions, etc... that took some time to write and "could" maybe be of interest to some users.

There are quite a lot of changes to revert, so it's probably going to take a little bit of time.

Thanks a lot @ressa!! 🥳

This is another big one 😅

Another very nice improvement on top of the disable sticky behavior added previously in ✨ Drupal admin_toolbar disable sticky/fixed state. Is there any option to do this? Active .

After fixing a very minor ESLINT validation error in one of the JS files, by adding a line return, since all the tests and jobs were still passing 🟢, I went ahead and merged the changes above at #36.

If more adjustments need to be made to the form, the doc or CSS we should always be able to address different issues and tickets as they come. 😅

Marking issue as Fixed for now.

Once again, I'm very grateful for your help through this process @ressa, testing/reporting back, reviewing, providing advice on the best or most simple options/solutions in a very timely and constructive manner 🙏
This feature could never have moved at this pace without your great help 🙂

Let's get a few more important tickets in before we could release this great new feature in a stable 🥳
Thanks again!

After doing a bit more testing, I wouldn't really be opposed to removing completely from the form the sensitivity and interval parameters....
Do you think we should just get rid of these settings?

Thanks @ressa!

I think I made all the changes you suggested above at #18, but mostly:

• Changed 'Timeout' default value to '500' and
• Moved 'sensitivity' and 'interval' to an 'Advanced' section collapsed by default.

 
I added a bit of description text to the advanced collapsible section to sort of warn users:
They should know what they are doing if they change these settings 😅
 
Otherwise all the tests still seemed to be passing 🟢 and my tests locally with big_pipe as well 👌
 
Let me know if you could spot anything else 🙂
Thanks again for the great help!

Thanks a lot @ressa for the great help with the CSS, reviews, testing and advice! 🙏

I've tested the CSS changes suggested above at #33 and they seemed to work very well 🥳

I've rolled them in the merge request, in the latest commit, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/123/di...

Mostly, I increased slightly from 200% to 220% since we could still see the small shadow of the menu at the top of the window (not sure if that was intended or not?! 😅).
I've done some tests as well with other modules adding toolbar items to make it wrap to the next line and with 220% it still seemed to work.
I've done a quick check and found the computed height in my browser was 40px, so 220% would be 88px, which "should" be enough to cover the height of the toolbar and its tray to make it exit the screen completely.

I've done a bit of testing locally and found the animation is pretty cool and works very well 🥳

I've added a small change to fix another JS problem I found after refactoring some of the hoverIntent and hover JS code in:
https://git.drupalcode.org/project/admin_toolbar/-/commit/4cbcb8d2df2a1b...
This issue would appear when using the focus key to navigate menu elements, then hovering the menu again: expanded menu items would still stay displayed.
 

I like the "sliding in from the top" effect, but did have to limit to only Horizontal mode, since it breaks Vertical mode ... Could this work?

I think it's completely fine: A quick search for vertical in module's code base brought up a single occurence:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/css/admin.to...

.toolbar .toolbar-tray-vertical li.open > ul.toolbar-menu.clearfix {
  display: block;
}

Which would seem to indicate none of the logic of the admin_toolbar module, whether hoverintent or sticky behaviors would apply to the navigation of the toolbar when it switches to vertical mode.

We could maybe create another issue to try addressing support for the vertical toolbar with a more generic approach, since at this point, it seems to be non-existent in the module anyway 😅.

Back to Needs review for more testing and feedback on the latest changes! 🙂
Thanks again!

See the Admin Toolbar module for the solution: 🐛 Some mandatory parameters are missing ("source") to generate a URL for route Active .

Thanks again @ressa! 🙏

I've taken a closer look at the example you sent above at #30 and it certainly helped 👍

I tried to add the variable delta to define a minimum distance in pixels to record a scroll and change the visibility state, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/123/di...

          // The delta is the minimum amount of scroll in pixels before the
          // toolbar is hidden or shown. This is to prevent the toolbar from
          // flickering when the user scrolls up and down quickly.
          const delta = 10;

The comparison is based on the abs() function, so it goes both ways: up and down:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/123/di...

            // Ensure user scrolled more than delta. The abs() is used to
            // enforce the delta distance in pixels in both directions: up/down.
            if (Math.abs(lastScrollTop - scrollTop) <= delta) return;

 
In this change, the delta was set to a value of 10 pixels, as suggested at #25.

I have tested manually and couldn't really see a difference with a low value (which is good), but with a value around 80 I had to scroll twice (mouse wheel) in the same direction before triggering the change of visibility, which seemed to be working as expected.

Lastly, I tested with the commands you provided above as well at #25 and they seemed to work as well 🥳

// Nothing happens.
window.scrollBy(0, -9);
// Toolbar is visible.
window.scrollBy(0, -11); 

 
Since the jobs and tests all seem to still be passing 🟢, moving issue back to Needs review as an attempt to get more feedback and reviews on the latest changes.
 
Any feedback, reviews or comments would be greatly appreciated.
Thanks in advance! 🙂

Thanks a lot @ressa, once again for all the advice and help on this big feature 🙏

I've just pushed a big round of updates which should include all your comments above, plus all the points from #14:
✅ Fixed all your comments above, thanks a lot!
✅ Greatly improved the form display by moving around form elements and wrapping all hoverIntent configuration related fields into fieldset and details (collapsible) wrappers. Added field descriptions, suffixes, prefixes, etc... to provide more information for non-tech savvy users and make the form overall a bit more user friendly.
✅ Added support for hoverIntent settings: 'interval', 'sensitivity' and 'timeout'.
✅ Added basic Function Tests coverage by checking the configured hoverIntent settings values are properly added to the 'drupalSettings' JS variable of the page.
✅ Refactored hoverIntent config update hook ==> TO BE TESTED (manually), if possible.

I've done a few tests with the form and personally found it much easier to understand with these changes 👍
See a screenshot below:

 
I would appreciate some feedback on the changes to the Settings form, the section wrappers (fieldset/details), field labels/descriptions, the added fields sensitivity and interval, etc...
 

Since all the tests and jobs are still passing 🟢, moving issue back to Needs review as an attempt to get more feedback and reviews on the latest changes.

Feel free to let me know if you catch anything else and your overall feeling on the latest changes, I would certainly be open to making more adjustments where needed 👌
Thanks in advance! 🙂

Thanks a lot @ressa!
I've got this issue on my list and had planned on updating the issue very soon...

Thanks for the link and advice!

Thanks a lot everyone for your great help on this issue! 🥳

Thank you very much for making the steps very clear in the IS:
Indeed, I was able to reproduce the issue and make a bit of digging around the impacted piece of code:

It seems the piece of code removed in MR!77 stems from an initial request in:
Issue #2991056: Always hide active menu item's navigation on mobile → ,
with commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/c1e705ff67b6d5...

which would seem to have "broken" module's previous JS behavior and led to:
Issue #3172430: Hide the navigation menu on mobile properly →
with commit:
https://git.drupalcode.org/project/admin_toolbar/-/commit/6dd6516
 

We can't be sure whether this change is going to break the expected behavior on some sites, but since this piece of code is currently causing an issue it is probably safe to assume, these changes should be reverted for now.

Perhaps the way this feature was initially introduced in the module should have taken a different approach?!
Maybe it could have been added as a form setting or in a sub-module, for example.

Personally, I don't think it would be a good idea anyway to have a different behavior between larger and smaller devices.
Additionally, if this was the case, we would probably want to alter JS variables set in the Local Storage by the Core Toolbar module, instead of changing the DOM elements of the page.

Again, we wouldn't be closed or opposed to discussing the reverted feature again, if anybody would still be interested in working on this feature.

Since the jobs and tests were passing 🟢, I went ahead and merged the changes above at #10.

Marking issue as Fixed, for now.

Thanks again everyone for the great work and contributions! 🙏

This task is a consequence of the api change introduced in Drupal Core 10.2, described in:
Change record: _drupal_flush_css_js() is deprecated and replaced by \Drupal::service('asset.query_string')->reset() → .

Based on the rector patches above at #2, the code of flushJsCss was refactored with backwardsCompatibleCall to support deprecated function _drupal_flush_css_js for versions below core:10.2.

PHPSTAN: Ignored error message:

Unable to resolve the template type Deprecated in call to method static method Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall()

Since all the tests and jobs were still passing 🟢, I went ahead and merged the changes above at #7.

Marking issue as Fixed, for now.
Thanks!

dydave → made their first commit to this issue’s fork.

Closing as a duplicate of 📌 Automated Drupal 11 compatibility fixes for admin_toolbar Active .

Fantastic! Thanks a lot Derek (@spiderman) for the kind and prompt reply, once again! 🙏

Alright, alright! I'll just back out if you don't mind!! 😅

I can see the module is in super great hands! You've got a great plan! 👌

So I'll just go back to fixing my stack of tickets and modules!! 😅
(I've got so much to do ...)

Feel free to ping me directly at anytime if you need anything, I would be happy to help the best I could 🙂
Thanks again for all the great work and keeping this module well maintained!
Cheers!

I'm seeing lots of other modules doing that (dropping version below 10.3), see for example :
https://www.drupal.org/project/commerce_stripe →

@mparker17 let's go?!

Is there anything else you could see that could prevent this issue from moving forward?

Personally, I don't feel concerned at all with this issue and change since all of the project I'm currently working on, or managing are all above 10.3 with recent versions of PHP (8.3.19 security patch).

If a customer came to us with a Core version lower than 10.3 (9.x or 10.x), I would recommend upgrading core and modules + consequently upgrade this module based on the different versions (major/minor), which doesn't really sound like a problem to me.

What does the M of @mparker17 stand for?! 🙂

Just my personal humble opinion.
Cheers !

Thanks @makbay!

I didn't see this: 📌 Deprecate in favor of the CKEditor 5 Plugin Pack RTBC

Please don't use this module anymore it should be marked deprecated.

Maintainers have agreed to move developments to contrib module CKEditor 5 Plugin Pack → , which now provides the same features.

This issue is most likely never going to get fixed and so are all other issues in the project's tracker.

This module should be considered as Abandoned.

Thanks @mparker17 for the prompt and kind reply, once again, it's greatly appreciated.

Sorry I didn't read carefully your previous comment (lack of time and finished work late😅).....

RE: #23

Note that, to my surprise, I found the new additions to the menu link data gets duplicated, given the current production code in the merge request and/or the example code added in structure_sync.api.php in this merge request.

Thanks a lot for raising this point.
🟢 I checked and indeed, this is the intended behavior: The API document demonstrates how various attributes and keys could be modified for a menu link item. In other words, the hooks don't only allow to modify the values in the options array, but also any potential key or attribute that users would like to add to a menu link item.

Do you think this could be misunderstood, unclear or misleading for some users?
These are "just" basic examples without necessarily any specific purpose in mind to illustrate how the parameters could be altered.

Thank you very much for taking this into account in the tests: it wasn't absolutely necessary, but it gives a good example of how the hooks could be implemented with more generic parameters.

Thanks a lot for getting this over the line @mparker17! 🙏

The tests look very great! 👍

I've done an initial read/review of the added MenuLinkExtraConfigTest Test class and more particularly its testExportAndImportWithCustomData Test case and it seems to do everything mentioned above at #20.

✅ The test module with all the new alter hooks structure_sync_hook_test is properly enabled.
✅ Test export_alter: Set a value for a menu link options' key 'structure_sync_hook_test_EXPORTKEY', export it and check the value was exported properly.
✅ Test update_alter: Update the value of the menu link options' key 'structure_sync_hook_test_EXPORTKEY' in the exported config data, import the modified data with the test value and check it was merged properly with the existing menu options.

The tests seem to be passing 🟢 which is great!

Not sure if you wanted to test the import_alter hook though, I "think" this one would need maybe destroying a link, which could be re-imported/re-created with the correct options (?!).
But that would just be an extra nice to have 😅

I hope I didn't misunderstand some of the code and that this review will be useful, since I mostly just read it...

I left another small comment in the MR, but overall it looks very good!
Thanks again very much for the great work! 🙂

Seems there are some activity around this topic and issues, see 📌 New Toolbar Roadmap: Path to Beta & Stable Active .

OK, yes, thanks @mparker17 and @spiderman!

Dropping support for versions below 10.3 should be fine as well, just like it's suggested in the MR:
https://git.drupalcode.org/project/structure_sync/-/merge_requests/37/di...

I made the suggestion above at #4 since it would be the most immediate solution to get module's development unblocked .... Not excluding the possibility to further drop support, in another minor version... So essentially both would be possible successively.

But if you'd like to make a bigger version drop right now, that should be fine as well.

However, not making any decision is blocking the development of the project and preventing project's tickets from moving forward.

Thanks for finding this!

I've done a bit of testing and I "think" I was able to reproduce with the steps clearly detailed in the IS.

I need to do a bit of digging though to see where and why this code was initially added.

Needs a bit more time testing, but bringing this back to the top of the stack.
Thanks!

Following the recent resolution of parent issue 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active , thanks to @ressa's great help, we should now be able to take another look at this issue 👌

I rebased MR!99 and made a few adjustments to the merge request, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/99/dif...

But mostly:

Updated config name, field label, description and added basic states to hide field if hoverIntent is disabled.

The MR should be ready to be tested at this stage, but I think some work would still be needed:

• Probably tidying up a bit the Settings form, adding a fieldset, with a description text, moving a bit fields around, etc...
• Do we really want to set a step for the configurable integers (timeout, for example)? What about a max?
• Would we want to add more HoverIntent configurable settings, see HoverIntent Common Options:
  https://briancherne.github.io/jquery-hoverIntent/
  such as interval or sensitivity?
• Automated tests would need to be added to check the settings variables are correctly added to the JS of the page.

 
Any other changes that anyone could see?

Otherwise, I tested locally MR!99 and it works very well: increasing the timeout slows a bit down the interactions with the menu, which could be very practical for certain users 👍
(I tested 750 and 1000)

Since the tests and jobs all seem to be passing 🟢, moving this to Needs review as an attempt to received more feedback to the points above, in particular some help with the field wording, descriptions, options to implement, etc...

Any comments, suggestions or feedback would be greatly appreciated.
Thanks in advance!

Thanks a lot @ressa for the great help on this issue! 🥳

This is definitely a big one with a lot of code clean-up, in particular for the JS files with a rewrite of the hoverIntent feature with standard Drupal JS API and as little JQuery as possible, while fixing all ESLINT errors in these files.

After your confirmation, I sneaked in a last commit to fix a few minor ESLINT validation errors, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

Minor changes with indentation and line breaks.

I gave the module a last round of manual testing locally, with big_pipe enabled and since all the tests were still passing 🟢 I went ahead and merged the changes above at #25 ! 🥳

Let's get to the fun part now 😆
We should now be able to add extra config parameters for HoverIntent: ✨ Make un-hover delay configurable Active

Thanks again very much for all the great help testing/reporting and super constructive advice on this issue @ressa! 🙏

Alright then @ressa! 🥳

Looks like we're all good then ! 👌

I've made the necessary code changes in the last commit, all jobs and tests are still passing 🟢
See commit: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

Everything described in your comment above at #22 is now implemented in the merge request, in other words :

Users upgrading from the previous stable to the next will only have the HoverIntent feature disabled, if they specifically selected it to be disabled on the admin_toolbar_tools form, before running the upgrade.
All other users, with only the admin_toolbar module enabled (admin_toolbar_tools disabled) or admin_toolbar + admin_toolbar_tools enabled (Enabled HoverIntent), will have the HoverIntent feature enabled.

I have just tested the upgrade with only admin_toobar enabled and after the last commit, it is working as expected:
HoverIntent enabled, after running DB updates.

Fresh installs will have it enabled by default.

I "think" we've really given all the different aspects a lot of thoughts and documented the different choices thoroughly in this issue 😅

Back to Needs review so you could take another look 👍

Thanks again so much for the great help !

OK, I think I understood this feature better yesterday:

In 3.5.3 and initially, I "think" what was initially planned for this feature is that the HoverIntent functionality would only be available if users enabled the admin_toolbar_tools module, see:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
If the admin_toolbar_tools module is not installed, then its config setting (hoverintent_functionality) doesn't have any value and therefore loads by default the Basic hover JS behavior.

So we would be modifying the initial logic by moving the setting out of the admin_toolbar_tools module, which should allow users to enable the HoverIntent setting just with the admin_toolbar module enabled.

Which I "personally" think is an improvement for the module, especially given your feedback on the smoothness and flexibility of the plugin.

The way this feature was initially implemented is quite confusing, because part of the logic went into admin_toobar and part went into admin_toolbar_tools.....
I "think" the way this feature was integrated to the module was to consider the HoverIntent as an "Extra" functionality, added by enabling the sub-module admin_toolbar_tools.

We have different options:
1 - Keep the current behavior and move the related logic from admin_toolbar to admin_toolbar tools (JS library, display logic in the admin_toolbar.module file), to have everything in one place.
2 - Change the existing behavior to allow admin_toolbar to fully control this setting (move the setting over to admin_toolbar).

Given the amount of tests and usage of this feature by users, we "should" be able to consider it should be stable enough to be moved to the "global" configuration of the module, and thus:
My preference would go to the option 2, which is currently implemented in the merge request: It would make sense to allow users to enable this feature without necessarily installing the admin_toolbar_tools module.

One thing would remain though if we wanted to be fully ISO with the previous logic: we would have to disable hoverintent by default, for users having only the admin_toolbar module installed after running DB updates.
In other words, users currently only running admin_toolbar and not admin_toolbar_tools, are using the standard Hover default behavior.
So after running the hook_update from this MR, the enable_hoverintent setting should be disabled and users would have to explicitly change the value in the form to select the hoverIntent behavior.
That is ... only if we really want to be ISO with the current behavior.... this is also open to discussion and we could just go ahead and enable hoverintent by default for these users as well.

That's the only comment I had left on the MR, concerning the code here:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

Otherwise, new users installing admin_toolbar, would be using the HoverIntent plugin straight away (default setting, same as admin_toolbar_tools).

But it was important for me to fully clarify the change for this feature.

Thanks in advance for your feedback and comments!

Super helpful feedback @ressa, once again! 🥳

Looks like we're doing some much needed clean-up here and I also don't really understand how the module could have worked with big_pipe before, without the correct dependencies to core JS libraries 😅.

I think some of the clean-up work we've been doing and enforcing certain job validations is starting to pay off 👍

For example, raising the PHPSTAN validation level allowed me to catch a wrong test I was doing in the added code of the hook_update function, which I was able to modify in the last commit.

I've given a round of tests as well, locally, for the update function and it would seem to be working fine/as expected:
Moving the old config value to the new config.

About the description, it is a difficult concept to explain in a few words

The description you suggested is much clearer, thank you very much for taking the time to make it easier to understand for non tech-savvy users, that's a very difficult exercise 🙏
I've made the change to the merge request and the field should now display properly with the new cleaner description.

Let's keep in mind, this field might further evolve soon, when we start adding more hoverIntent config parameters, in particular in child issue: ✨ Make un-hover delay configurable Active
So we should still be able to make more refinements to the field labels/descriptions in the future.

Lastly, concerning big_pipe:
I'm also very happy we could clean-up all this part properly:
All the JS files related with this feature: admin_toolbar.hover.js and admin_toolbar.hoverintent.js
were moved to Vanilla JS as much as possible, with a single JQuery call to the hoverIntent plugin,
fixed all ESLINT validation errors as well and
wrapped in Drupal behaviors with calls to Core once, to ensure their code would only be executed once.

This started prompting errors and problems when enabling big_pipe (calls to once), which allowed identifying the libraries could be loaded in the wrong order due to missing dependencies.

Concerning the Tests: I'm not sure, but I would tend to "think" PHPUNIT Tests don't necessarily run or enable the big_pipe module by default. Or maybe there could be other reasons as well...
But this one is definitely a module that needs to be enabled when doing manual testing: I'll pay much more attention in the future.

Feel free to give the MR another round of review and let me know if you spot anything else 👌
Otherwise, I've done quite a bit of testing myself as well and think the changes should be pretty much ready to go.

Thanks again for all the great help! 🙏

Quick follow-up on this issue:

Starting from level 3, I gradually raised the level and each time fixed all the errors prompted by PHPSTAN, until reaching level 6.

Fixed all validation errors, mostly:

• Updated Doc comment blocks return types.
• Updated Doc comment blocks param types.
• Added inline @var types definitions.
• Changed 4 lines of code, to add elvis operators and reorder chained calls.
• Caught a wrong variable passed in assert function in a Test class.

 
Added project's phpstan.neon file to raise validation level to 6 and ignore certain error messages.
 

Very few lines of code with very minor changes were impacted in the merge request.

Since the Tests of the MR all passed 🟢, I went ahead and merged the changes above at #3.

The configuration change was made on Gitlab CI by adding the file: phpstan.neon, forcing rule level to 6.

This should be helpful keeping module's code properly maintained with the evolution of the different Core versions, APIs and PHP versions.

PHPSTAN validation is currently not required to merge.

Marking issue as Fixed for now.

Thanks!

dydave → created an issue.

Thanks a lot @mparker17 for all the nice and prompt replies, it's greatly appreciated! 🙏

Do you feel comfortable writing the first draft of the test? If not, I can try, and pass it to you for refinement!

I'm very sorry, but frankly, I don't think I would have the time right now:
I'm working on improving the Admin Toolbar → module these days and there's quite a lot to do actually 😅
In particular updating some of the JS libraries.

I've got plenty of tickets, MRs and support requests to answer already 👌

Not to mention several D11 compatible tickets I'm still trying to push 😅

But I would be happy to review or test any code produced 👍

Thanks again very much for keeping this module well maintained!

OK @ressa:

• Reverted the default value to be: HoverIntent Enabled (checked) / Exactly as it is currently in 3.5.3.
• Fixed the big_pipe issue by changing the JS libraries dependencies.
• Updated the enable_hoverintent checkbox label and description to be a little bit clearer, but feel free to make suggestions:
  

  ✅ Enable HoverIntent
  Provides enhanced control over the mouse movements and interactions with the menu items of the Admin Toolbar.
  Disable to use module's default basic javascript behavior.

Sorry for the scope creep again, above at #16:
Changing the default value along with the way the plugin file is installed is definitely out-of-scope in this issue 😅

Tests are still passing 🟢 Back to Needs review 🤞

Feel free to let me know @ressa if you catch anything else, I would be glad to look into it as soon as possible.
Thanks again very much for the great help!

Additionally: Automated testing on Gitlab CI allows running PHPUNIT Tests and PHPSTAN code validation for the 3 currently supported major core versions.
See the changes suggested in issue 📌 Gitlab CI: Broaden Tests coverage and require jobs to pass Active , with MR!47:
Build pipelines: https://git.drupalcode.org/issue/structure_sync-3517495/-/pipelines/465983

See the different jobs for the different types of supported versions.

This ensures a better automated testing coverage and can help reporting many deprecation errors so maintainers can make module's code evolve with the different Core version API changes.

In other words: with all the Gitlab CI optin variables enabled, we should be able to provide a pretty good and standard support coverage of the different versions required by Core.

Quick follow-up on this issue:

Created initial merge request MR!47 above at #2 which should enable complete Tests coverage and code validation on Gitlab CI.

These changes should allow the module to exploit and benefit from all of the current features and configuration options provided by the Drupal templates for Gitlab CI, available at this moment.

The MR currently fails on PHPCS validation job, see parent issue 📌 Fix PHPCS validation job errors Active .
So, once the changes from the other MR are merged, this MR should pass all jobs and Tests correctly.

Since all Tests are passing 🟢, moving issue to Needs review:
https://git.drupalcode.org/issue/structure_sync-3517495/-/pipelines/465983

The configuration in this MR is really only a suggestion, it is left at the maintainers appreciation to comment/uncomment/remove any sections added in the changes.

Feel free to let us know if you have any questions or concerns on any of the changes in the merge request or this issue in general, we would surely be glad to help.
Thanks in advance!

When the following core version will come out, same thing:

Create a new major branch and drop support for the requirements of the the oldest version.
Supporting 3 major core versions in a module major version gives a large widow for users to upgrade core and other contrib modules in different steps and flexible order.

This is what I understood maintaining modules and reading the following post:
Drupal module semantic versioning for Drupal core support

Could you please help us make the suggested changes so we could keep moving forward with other issues and update pending merge requests to point to the 3.x branch?

Thanks in advance for your feedback!

Thanks @mparker17!

I would recommend doing the following changes:

1 - Create the new major development branch: 3.x
2 - Drop support for all core versions below 9.5
3 - Drop support for all PHP versions below 8.0.0

The requirements would then become:

core_version_requirement: ^9.5 || ^10 || ^11
php: 8.0

Changes would also have to be made to the composer.json file, see:
https://git.drupalcode.org/project/structure_sync/-/blob/2.x/composer.js...

Let me know if you would need any help putting together a corresponding merge request, we would surely be glad to help!
Thanks in advance!

Quick follow-up on this issue:

Created initial MR!46 above at #2 which fixes all PHPCS errors.

I just executed locally automated fixes with phpcbf and all the errors were fixed.
Most of the errors where just a missing nullable operator on some of the variables Type declarations, see the merge request for the detailed changes.

Since all Tests are now passing 🟢, moving to Needs review and bumping to Major: This issue is preventing other merge requests from being properly tested.
Additionally, we are unable to know whether pending merge requests could be introducing Coding Standards errors.

We would greatly appreciate if a maintainer with write access could take a look at the merge request and let us know if any more work would be needed.

Thanks in advance!

dydave → created an issue.

dydave → created an issue.

Thanks @mparker17!

RE: #17

The Structure Sync maintainers prefer to accept merge requests that have passing automated tests. If you need help writing tests, please ask: I would be happy to help!

Is this something you could help adding to the module?

We could perhaps:

Add a small Test sub-module that would be enabled in a Test class which could trigger implementations of the hooks introduced in the merge request:
https://git.drupalcode.org/project/structure_sync/-/merge_requests/20/di...

• hook_structure_sync_menu_link_export_alter
• hook_structure_sync_menu_link_import_alter
• hook_structure_sync_menu_link_update_alter

 
Extend or build on top of existing FunctionalJavascript Test class: FunctionalJavascript/StructureSyncMenuLinksTest.php
https://git.drupalcode.org/project/structure_sync/-/blob/2.x/tests/src/F...

So all these hooks could be triggered by the operations tested in this class.
 

Note: I've noticed there are PHPCS errors currently in the MR, unrelated to the changes from this ticket.

We would certainly be glad to hear your opinion on the suggested Testing approach.
Otherwise, any help testing, feedback, reviews, questions or comments would be greatly appreciated.
Thanks in advance!

Thanks a lot @ressa!

Super helpful on all points! 🥳

I cracked the riddle: It's BigPipe!

Great job! That's super helpful and indeed, I generally disable big_pipe since it currently sort of breaks the collapsible on devel:dpm with Symfony vardumper.
So that's totally on point and I'm going to look into this ASAP.

I think I understand why the hoverIntent plug-in cannot be used by default ...

First of all, I'm completely open to discussion and making different choices.
This has not been completely settled yet.

But, Technically speaking, I was brought to think this approach would be preferable because:

If we're looking at moving the hoverIntent plugin (whether Vanilla JS or JQuery) out of the code base, to be added as a dependency required in the composer.json file, we have to assume there might be cases where the dependency might not be met.

See #3513588-16: Update the jquery-hoverIntent JS library → :

The admin_toolbar module is so popular that we need to support installation from a tarball, for example, people using the ZIP file to install the module manually and not composer, see for example:
https://www.drupal.org/project/admin_toolbar/releases/3.5.3 → (Download zip)

In this case, they would not be able to have the JS file downloaded at the same time and therefore the module would be broken.

In other words: Not all users may be using composer to install the module and therefore might not have installed the hoverIntent plugin. In such cases, they would have to do it manually and place the file at a specific location, see for example the Colorbox module installation steps:
https://git.drupalcode.org/project/colorbox/-/blob/2.1.x/README.md?ref_t...

Many other JS or JQuery related modules will have such kind of logic and also special messages displayed on the Status report page.
Additionally, any types of settings related with the missing libraries would be disabled to fallback on a default or even disabled state/files.

This is why I would "personally" suggest that we set the default behavior to the standalone admin_toolbar default vanilla JS behavior, without any external hoverIntent plugin.
 

But I'm open to anything really if you have better ideas in terms of UX: Let's try thinking a bit further:

If we set the hoverIntent library as the default, with the checkbox ticked by default, as it is currently on the Settings page, we could add an extra check upon display to check whether the file exists, in which case we would attach the plugin library, otherwise we would just fallback on the default..... something like that?!

In other words: if the plugin file doesn't exist, the default file would always be loaded.
+ Warning/Info message on the Status report page
+ Info message on Settings form + field disabled

I do know that some contrib modules, like Leaflet and Collapsiblock, simply bundle the JS library inside the module, to avoid loading external resource, and the module "just works". Is this not possible for Admin Toolbar as well?

Sure, no problem, that's also another option:
We should just not do this 😅 Moving the plugin out of the code base, see:

I have reviewed your changes and thought a little bit more about it and we're not going to be able to move the JS plugin out of the code base.

This is definitely not as straight forward as I would have thought....

I'm definitely open to suggestions @ressa 🙏
 

For the time being: We're going to keep the same logic as currently: hoverIntent plugin enabled by default (checked).
IF later we find a good way to move the plugin out of the code base and decide it is a better choice than keeping the file, then we will change the default value in the corresponding merge request 👌

I'm reverting this change for now!

I'm getting back to work on the MR... this will need more time to get reviewed and tested properly 👍
Thanks a lot!

Thanks a lot @ressa!!

I'm going to give this a clean round of tests later today with a fresh D10 and D11 sites.

I'll definitely get back to you on this as soon as I have a bit of time.... I'm gonna be stuck in clients meetings this afternoon 😅 (joy!! 😅)

But I'll circle back on this ticket as soon as I am free 👌

Cheers! 🙂

Looks good @ressa!

Yes! It looks accurate .... not sure about D11 ... You're right... I haven't tested that ...

Try playing with the form options at admin/config/user-interface/admin-toolbar Check and uncheck, flush cache (just in case), and test the display of the menu... see if it works ....

I'll give a round of tests again on a fresh D11 install as well...

If cache flushes need to be added, that's part of the changes to be added to the MR...

Try doing a bit more tests and report back ... I'll do the same this afternoon quickly... and ask some colleagues as well.

There's still some work needed indeed, with the checkbox label and description also.

So any feedback at all would be a great help!
Thanks in advance!

RE #7 for the checkbox field label:

Bear in mind this is going to show/unfold the other config fields which will be added in the child issue to make the "hoverIntent" parameters configurable ... so one or more fields will be added and visible there when the checkbox will be checked.

I've also changed the default behavior on install to use module's default JS standalone script.
In other words, currently, the default is JQuery hoverIntent and after this MR is merged, the default will be the simple standalone JS script.

The advanced hoverIntent animation will have to be selected by users specifically.

The reason for that is that the default shouldn't rely on an external JS plugin dependency.
If the script is not there or the dependency is not met for some reason....

By default, the module should be able to work fine without anything else (ie. its own script).

I've also got an answer line-up on the scroll issue, as well so I could get back to you on this ....
Just need a bit of time to put down everything in the ticket 😅

RE: The field description and label:

What does this field do?

Technical explanation:

There are 2 scripts to handle the hover menu animation to unfold the menu items:
1 - Default: is custom JS produced by and for the admin_toolbar module:
Standalone script: https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...
Which doesn't require any external library to work ... just the usual Core libraries.

But this animation is a bit "dry" in the sense, the JS doesn't provide any feature on the time delay to hide the menu, to show, etc...
It's very basic and simple, the display and hide are just instantaneous.

2 - HoverIntent: is the JQuery HoverIntent plugin:
We've got a small piece of JS in the admin_toolbar module that builds on top of the JQuery plugin:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...
See how it call the JS function hoverIntent.
See recent issue: 📌 update hoverIntent Active
Plugin URL: https://github.com/briancherne/jquery-hoverIntent

This is much more flexible, with a lot of parameters, see the demos here:
https://briancherne.github.io/jquery-hoverIntent/

Thus allowing to do nice things such as requested in child issue ✨ Make un-hover delay configurable Active
But a lot more parameters "could" potentially be configurable.

I think it's very nice to have this feature in the module, but it needs to be a bit refactored currently, see my explanation in the IS.

Plus the wording of the field is not clear at all ....

So if you have any suggestions, they would surely be very welcome! 🙂

I definitely need help moving this ASAP please .... this is a big move that's going to unblock a lot of other tickets I've got lined up for a lot of improvements and code refactoring/clean-up.
As a heads up: We're going to move to Vanilla JS entirely and replace this JQuery plugin with its Vanilla JS equivalent: 🤫
https://github.com/svivian/sv-hover-intent-js

See the code example on that page for some of the parameters that "could" be configurable "out-of-the-box" 🥳
(without "too" much work)

This is coming tonight ... during my contrib time!

Please help!! 🙏

Also, make sure you have the correct settings enabled on the config forms in the backend after running the DB updates, if you're updating from a stable or branch 3.x ....

Code :
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132/di...

I've tested this a bit, switching from 3.x to this feature branch 3516995-move-the-hoverintentfunctionality and run drush updb, and everything seemed to work fine ...

But I'd like to get more testing feedback on this, if possible, as well ....

This is a big change @ressa for refactoring and clean-up.... we're going to feel much better once this is IN 😅

Thanks you so much @ressa !!

Did you try flushing the cache ?!

Very strange, my colleagues gave this a round of tests and reviews and it was working ...

Also, watchout, you need to run db updates after applying the patch or switching to the branch.

Also, I don't know what hoverintent is about ... so perhaps this sentence could be expanded, and briefly describe what it does?

Totally agree with you!
I didn't want to change this so far, since it's going to break all translations ....

But since we're going for a new minor 3.6.0 and are already introducing BC breaking changes (see: #3514075-5: unsafe usage of new static() → ) and this change in particular in this MR:
Got me very worried and I made a lot of research to see how to handle changing a config or renaming it ....
That's also potentially BC breaking ...

So we're going for a new minor, definitely !

I'll create tonight the META issue: Road to 3.6.0, as you recommended previously.... Now the release plan is clear to me !
Let's go and break thinks 😅

I'll get the wording updated for this setting, definitely, based on your suggestions...

But please if you could: Try forcing a bit more the tests to work on your ENV locally ...

Everything seemed to work for my colleagues.

Stay tuned for rounds of changes from tonight and over the weekend !

Thanks again so much!

Any chance anybody here could help testing the merge request in parent issue 📌 Move the 'hoverintent_functionality' setting to admin_toolbar Active ?
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/132

Once the other MR is in, we should be able to get this issue merged in quickly.

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

Yesterday, I contacted project Maintainer Mateu Aguiló Bosch (e0ipso) → ...
Let's see if we could get a reply.

Otherwise, I'll try contacting the other maintainer.

Thanks!

dydave → created an issue.

We'll see, but I'll most likely split this into a separate ticket to get the hoverintent_functionality moved out from admin_toolbar_tools to admin_toolbar and come back to this issue right after to add the state on the field added by the MR.

More coming in the next few days.
Cheers!

Thanks a lot everyone for the great work on this issue, it's greatly appreciated!

I've rebased/fixed a few conflicts that were blocking MR!99 and it should be good now.

I haven't tested the feature yet, but after a very quick review of the MR, it looks good overall, great job! 🙂

I think there are a few more changes I'd like to add to the MR, in particular, I think the whole hoverintent feature should probably be moved out from the admin_toolbar_tools module to the admin_toolbar.
This feature "should" work without the admin_toolbar_tools module, so the config variable hoverintent_functionality should be move out to admin_toolbar, impacting the schema, the settings forms and probably the install files of the module (to update module's configuration on all sites).
I'll then add a form state to the added hover_intent_timeout config field to hide or show if the functionality is enabled or disabled.

Lastly, we'll mostly likely need to add Functional tests for this:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/tests/src/Fu...

More work is needed on this, so I'm setting this to Needs work and will circle back on this in the next few days: I'm going to need a few hours to get this properly fixed.

Bringing this one back up to the top of the list.
The good news is that the changes at this point seem pretty straight forward, therefore there are great chances this feature could be merged in the coming days/weeks.

At which point the MR will be set back to Needs review and we will need you help testing and reviewing again.
Thanks again!

Hi Kristen (@kristen pol),

I confirm the issue: The module doesn't define any menu link in the administration menu.

Currently, the module is only accessible through the "Configure" link on the module's extend page, or as a tab (local task) on the User management page (People).

It would be very practical if the page could also be accessible through the administration menu, which most users see through the admin_toolbar module.

I haven't tested the changes from MR!5 but looking at the changes, they would seem to accurately address this issue.

Any feedback, comments or questions would be greatly appreciated.
Thanks in advance!

Thanks a lot David (@davidiio) for all the great work, perseverance, patience and positive feedback! 🙏

I'm going to be quite busy until the end of the week, but I wanted this ticket to go back to the top of the list and I'm definitely getting back to you on this ASAP.

I wanted to get this in, before creating a new release, but I thought many users were waiting on the D11 compatible stable release....

This is going to move forward for sure and I'll try to get back to you on this ASAP, when I can find some time.

Thanks again !

Created initial merge request MR!6 above at #2 with minimal configuration to support automated testing on Gitlab CI.

All PHPUnit tests seem to be passing so it should not be blocking any pending merge requests.

Thus moving issue to Needs review.

More changes to the Gitlab config file could be done in the future to broaden Tests coverage and/or require certain jobs to pass.

Any comments, feedback and questions would be greatly appreciated.
Thanks in advance!

dydave → created an issue.

Could this be a duplicate of 📌 Create Custom Permissions 8.x-2.2 release for Drupal 11 support Active ?

If you would like to help moving this issue forward and get the module compatible with Drupal 11, please help us test and fix the following issues first:

• 🐛 Permissions that depend on disabled modules prevent any editing/creation of other permissions Needs review
• 🐛 Module does not work with routes that implement _entity_access Needs review

Thanks in advance!

Drupal 11 compatible stable release created, see: 3.1.0 → .

Thanks everyone for the great help and in advance for your testing and feedback.

Thanks a lot Joery (@flyke) for creating this issue and documenting it very clearly, it's greatly appreciated.

The problem and solution suggested in the issue summary completely make sense, indeed.

Based on your suggestion, I created merge request MR!9 above at #3, which adds a dependency to the Core Toolbar module library for the ToolbarModel class, see:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/toolb...

I took the opportunity to add a few minor additional changes to the module's Tests class, see the merge request description for more details.

Since all the Tests were passing 🟢, I went ahead and merged the changes above at #4.

Marking issue as Fixed for now.

Feel free to let us know if you have any questions or concerns on any of the recent code changes or the module in general, we would surely be glad to help.
Thanks in advance!

Thanks a lot everyone for your help with this issue and contributions, it's greatly appreciated.

Based on the work from @ashwin.shaharkar above at #7, I created the corresponding merge request MR!8 at #9.

Note the composer.json file should not require a require section, since module dependencies and composer requirements should be evaluated based on the info file.

Since all the Tests were passing 🟢, I went ahead and merged the changes above at #10.

Marking issue as Fixed for now.

Feel free to let us know if you have any questions or concerns on any of the recent code changes or the project in general, we would surely try answering as soon as possible.
Thanks in advance!

Quick follow-up on this issue:

Since there were not "too" many issues to fix for each job, I was able to create a single merge request and fix all issues in the same ticket.

Created merge request MR!7 above at #2 with all the necessary changes to fix all jobs validation errors:
Fixed the Stylelint, PHPCS and PHPSTAN very quickly, since there were only a single error for each job.

Fixing ESLint took a bit longer, but using the --fix flag with the eslint command, fixed almost all the issues.
Remaining issues could be fixed manually without too much trouble.

Lastly, since all the jobs were passing, the Gitlab CI config was updated to require all of them to pass.

Since the merge request passed all Tests 🟢, I went ahead and merged the changes above at #3.

Marking issue as Fixed for now.

Feel free to let us know if you encounter any issues while using the module or have any questions or concerns on any of the latest code changes, would be glad to reply as soon as possible.
Thanks in advance for your feedback.

dydave → created an issue.

Thanks a lot for reporting this issue Bálint (@pasqualle), it's greatly appreciated!

This is a very common issue that I have found in many "smaller" modules, for example, recently, see:
#3507910-7: Offering to co-maintain Login Switch →

It's very confusing to users to understand what should be the correct development branch/version corresponding to the current stable based on its versioning....
Additionally, it's quite annoying for versioning tickets and creating merge requests.

While, it should be very simple and straight forward to fix.

That's why I completely understand and agree with this request.

Quick follow-up:

First, I checked whether there were still outstanding merge requests in the repository:
https://git.drupalcode.org/project/toolbar_menu/-/merge_requests
and found 2, which should be acceptable to modify manually by editing the MRs and changing the target branch.

Next, I went ahead and cloned the 8.x-2.x branch to 3.x and pushed to project's repo.

Then, I created the corresponding 3.x-dev → , which now appears correctly on the project page, along with the current stable (3.0.0).

Once the dev branch and release were created, I went back to the merge requests and changed their target branch, as well as the version in corresponding issues.

Lastly, I changed the repository's default branch to 3.x, at:
https://git.drupalcode.org/project/toolbar_menu/-/settings/repository

At this point, we should be able to move forward with any development work in the 3.x branch and step by step update all issues in project's tracker to match with the new DEV version.

Marking issue as Fixed for now.

Feel free to let us know if you have any questions or concerns on any of the recent code changes or the project in general, we would surely be glad to help.
Thanks again for you help and interest in the Toolbar Menu module.

I'm not seeing any mention of admin_toolbar in module's code base.

There has not been any further report of this issue since it was last answered, when Admin Toolbar is a very popular module.
Maybe the issue has been fixed on the Admin Toolbar side (?!).

Marking issue as Closed: Outdated for now.

Feel free to re-open this issue if you are still having the problem.
Thanks in advance!

This patch is a duplicate of 📌 Automated Drupal Rector fixes Closed: outdated .

Marking issue as Closed: Duplicate.

Thanks!

Thanks for all the work on this issue!

It looks like this was already fixed in:
https://git.drupalcode.org/project/toolbar_menu/-/commit/6e4d0ea71521470...
corresponding to MR!1
and issue 📌 Automated Drupal 10 compatibility fixes Fixed .

Marking issue as Closed: Outdated.
Thanks!

dydave → made their first commit to this issue’s fork.

Thanks everyone for the great help on this issue!

I've rolled in the changes from the latest patch at #8 in merge request MR!6 to get the work started on this issue.

I'm not exactly sure I understand how to reproduce the problem here, could somebody please provide more details?

For example, which type of Toolbar is used with the Gin Theme?
Any other potential modules?
Where is this class toolbar-menu-administration exactly used in the Gin theme? What causes the issue exactly?
(Any link code references pointing to the Gin repo)

Could we please get some screenshots as well?

Additionally, I'm still seeing the issue mentioned above at #3 with extra empty items, when enabling Gin, with any of the Gin Toolbar selected (see in Gin theme settings):

 
I'm seeing the same exact issue with any of the Toolbar menu items created and displayed in the Toolbar.

Could we try fixing this issue as well as part of a GIN Theme compatibility fix?

Any help, comments, testing and feedback would be greatly appreciated.
Thanks in advance!

dydave → made their first commit to this issue’s fork.

Quick follow-up on this issue:

Got maintainer's rights for the Toolbar Menu module \o/ 🥳

First, I wanted to make sure the module's Tests could run and pass on Drupal 11.

So, I added a Gitlab CI file to be able to run automated Tests and validation jobs for merge requests, see ( 📌 Automated testing: Configure GitLab CI Active ).
Then, I updated this merge request to be tested with all supported Core versions.

Since the PHPUNIT Tests were all passing 🟢:
https://git.drupalcode.org/project/toolbar_menu/-/pipelines/460931
So, I went ahead and merged the changes above at #9.

A new stable release compatible with Drupal 11 will be created in the coming days.

Feel free to let us know if you have any questions or concerns on any of the latest changes or the project in general, we would surely be glad to help.
Thanks again for all the help testing the module.

Quick follow-up on this issue:

Before getting any other MR merged in and/or moving forward with any change, it would be super helpful to go through the validation of all the jobs and automated tests.

Therefore, I went ahead and merged the changes above at XXX to activate minimal integration of the module with testing on GitLab CI, before starting to review and test any other issue.

We should be able to broaden Tests coverage and enforce certain jobs in other tickets, when needed.

Marking issue as Fixed for now.

Thanks in advance!

Thanks a lot Alberto (@avpaderno) for your reactivity and moving forward this request so quickly, it's greatly appreciated!

I've started merging in pending changes and will most likely create a D11 compatible release this week.

Thanks again for the great help!

dydave → created an issue.

Quick follow-up on this issue:

Updated the composer.json file as described in the issue summary.

Additional change:
Moved the PHPSTAN rule level configuration to the Gitlab CI config file since it is very simple and removed the phpstan.neon file that was added in commit:
https://git.drupalcode.org/project/image_link_formatter/-/commit/58e55af...

The changes were merged above at #3, marking this issue Fixed, for now.

Thanks!

dydave → created an issue.

No answer received from user for a very long time and no one else reported a similar issue.

Closing for now: Outdated.

Outdated issue: This won't get fixed.

Development and testing has moved to newer core versions and Gitlab CI.

Closing for now.

Outdated issue: This won't get fixed.

Development and testing has moved to newer core versions and Gitlab CI.

Closing for now.

dydave → created an issue.