Move the 'hoverintent_functionality' setting to admin_toolbar

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 things 😅

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!

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 😅

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: #3513588: Update the jquery-hoverIntent JS library
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 #3440852: Make un-hover delay configurable
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
(We're following the Drupal Core trend ==> Moving away from JQuery 😅)

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

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 #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.
(Only for new installs: Existing sites will keep their existing configuration, see the update hook in the MR)

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).

We should then be able to move out the JS plugin from the code base to a composer requirement (installed in libraries with composer) and I'll add checks on the settings form to check if the library is there for hoverIntent... if it's not there, we'll disable the checkbox and show some kind of warning or information message on the status report page, letting users know they can extend the module by installing the library manually if that's what they want.

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.

oh .... and also check the console output and potentially the logs to see if any error or anything that could help fixing this could come up 🙂

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

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

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!

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!

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: #3440852: Make un-hover delay configurable
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! 🙏

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!

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 😅

Once again, I made all these changes because I believe your suggestions are worthy of interest and for the better good of the module. I completely agree with all your suggestions, orientations and choices ==> Let's keep moving forward! 🙂

The only reason I could see that users would not want to have HoverIntent enabled by default after upgrading (just the admin_toolbar), is maybe for performance reasons, since we would be adding a small JS file to be loaded.... but I'm not sure how impacting that could be 😅

Back to Needs review so you could take another look 👍

Thanks again so much for the great help !

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: #3440852: Make un-hover delay configurable

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