- Issue created by @andresalvarez
- Merge request !124Update hoverIntent to version 1.10.2 and improve code formatting → (Merged) created by Unnamed author
- 🇫🇷France dydave
Thanks a lot Andres (@andresalvarez), once again for the great work and interest in the Admin Toolbar module!
The merge request looks fine 👍
But, could we try to maybe move the jquery-hoverIntent library out of the module's code base?
Maybe do something like the Colorbox module:
1 - Add the JS plugin to the composer.json file:
https://git.drupalcode.org/project/colorbox/-/blob/2.1.x/composer.json?r...
2 - Declare the plugin as a JS library:
https://git.drupalcode.org/project/colorbox/-/blob/2.1.x/colorbox.librar...
3 - Remove the file from admin_toolbar repository.This way, the plugin's code wouldn't be maintained in our module.
Could you maybe give this method a try and see if it could work?
Any help, feedback, comments or suggestions would be greatly appreciated.
Thanks in advance! - 🇨🇴Colombia andresalvarez Bogota
Hi, how are you doing?
of course, I've already checked the implementation
- 🇫🇷France dydave
Super nice Andres (@andresalvarez)!! Thanks a lot !!
You could maybe create a new branch/merge request and just leave the other one open, just in case we choose to go back to just upgrading the jquery-hoverintent library....
Really up to you.Glad the changes worked!
Could you please push them in a merge request?I should then be able to review and test the changes, so we could move forward with this issue.
Great work again! Thanks a lot! 🙂
- 🇨🇴Colombia andresalvarez Bogota
hello sorry I had not seen the comment I did was to use the same merger.
- 🇫🇷France dydave
No problem at all Andres (@andresalvarez)!
Do whatever you prefer!
Just please change the status (to Needs review, whenever you're done with the changes, so I could take a closer look at the merge request.
No worries.
Thanks again!
- 🇨🇴Colombia andresalvarez Bogota
perfect I will be attentive, anything else you would like me to help you ?
- 🇫🇷France dydave
Thanks a lot Andres (@andresalvarez)!
Please leave the MR with me I'll take a look later today, maybe in 4 or 5 hours, when I get a bit more time to do some testing and reviewing the MR.
I'll keep you posted for sure, if anything comes up 🙂
Thanks again for all the great help! -
dydave →
committed 750fc529 on 3.x authored by
andresalvarez →
Issue #3513588 by andresalvarez, dydave: Updated the 'jquery-hoverIntent...
-
dydave →
committed 750fc529 on 3.x authored by
andresalvarez →
- 🇫🇷France dydave
Thanks again Andres (@andresalvarez) for your help with the changes!
I'm very sorry Andres for getting out-of-scope 😖 at #4:
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.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.
It's my bad, but at least we know this will not be possible, at least for now: we need to think about this a little bit more and do a little bit more research to see how other modules handle this.
But it should definitely be in a separate ticket.
Therefore:
I have cloned your changes to a backup branch :
https://git.drupalcode.org/issue/admin_toolbar-3513588/-/commits/3513588...I have reverted your changes to the first commit (git reset), updated the JS file to be exactly the same as the target JS source file:
https://raw.githubusercontent.com/briancherne/jquery-hoverIntent/refs/ta...
Added the changes to the first commit and pushed to update the merge request.I think it's preferable to keep an exact copy of the source JS file for tracking purpose (comparing to the source).
Coding standards for this file don't really matter, since it should be excluded from ESLint validation, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/116/di...Please note the JS plugin file was last updated about 5 years ago:
Apr 27, 2020
https://github.com/briancherne/jquery-hoverIntent/blob/v1.10.2/jquery.ho...Therefore, it "should" be compatible with all the JQuery, JS, Browser, etc... versions supported by the admin_toolbar module (9.5+ currently).
I have done a bit of manual testing and all modules including Admin Toolbar Search seemed to work fine, as expected.
Since all the tests of the merge request passed 🟢 , I went ahead and merged the changes above at #15.At this point, this issue should be considered Fixed.
Feel free to let us know if you encounter any issues with this upgrade of the JQuery plugin or if you have any questions on this issue in general, we would surely be glad to help.
Special thanks to Andres (@andresalvarez) for your great help keeping the module well maintained! 🙏 Automatically closed - issue fixed for 2 weeks with no activity.