In #3411099: Create an administration UI for managing Navigation Blocks we added the ability for the user to manage the navigation sections/components/blocks. That implemenation was a copy of the core blocks implementation, with some tweaks made where necessary. Part of that copy was some Javascript that helps with the front end management of the Navigation admin UI. That copy brought with it use of jQuery, which the block system still makes use of. Note that the copy of the block admin Javascript came from the 10.2.x branch of core. Ideally, we would avoid introducing new Javascript that relies on jQuery.
Conver the Javascript that was added in #3411099: Create an administration UI for managing Navigation Blocks, that uses jQuery, to use vanilla Javascript and drop the jQuery dependencies.
NOTE: As this Javascript came as a copy of the block system Javascript from Drupal core, the work done here, would be very applicable to the core goal of reducing jQuery. Consider contributing the work done here back upstream to Drupal core #3052002: [meta] Replace JQuery with vanilla Javascript in core.
Code refactoring.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #4
finnsky commentedRemoved jquery from one file. Another seems larger. But still possible.
Comment #5
kostyashupenkoi have a question regarding this file https://git.drupalcode.org/project/navigation/-/blob/1.x/js/navigation-b... in terms of this task
This js file is almost identical to https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...
Is there a real reason regarding duplication of file? Can we just provide similar HTML markup than block.js expecting? By providing all the necessary classnames and table ID (maybe some other ID, dunno)
If we are not going to fully depend on block.js, maybe we can partially depend on block.js and instead of overwriting a whole file -> we can override only necessary methods / full behaviors.
And finally - this file contains some jquery prototyping, like $.fn.drupalSetSummary , and this kind of things is defined in https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/form.js?...
It's written on jQuery, so makes sense to nest it from core and don't override on the own javascript
Comment #6
kostyashupenkoSame question to `navigation-block.admin.js` file. It's almost identical to core's `block.admin.js`.
Comment #7
m4olivei@kostyashupenko you're correct that the Javascript is near identical. It's a copy and paste with "Block" replaced with "Navigation block". There are a couple of differences in that, Blocks are set per theme, Navigation Blocks are not. Also in #3412119: Restrict configuration of the Navigation bar footer items, the ask is to restrict administration of Navigation blocks to only the Content area, which would further adjust the Javascript.
The general idea here is to have an independant concept of a "Naviagation Block". They are "block-like", but there are differences. Thus we want to manage our own Javascript independent of the core Block system so as not to be coupled to it as much as possible / practical. In addition, with this going into core some day, I believe (don't quote me) that we shouldn't be introducing new Javascript with jQuery.
Hope that makes sense.
Comment #8
finnsky commentedThere are some efforts in core in this direction.
Comment #10
bronzehedwickComment #11
bronzehedwickI pushed a patch that should address all the jQuery to vanilla conversions. Looking for help testing, as I have less knowledge of all the edge cases and test procedures. Thanks!
Comment #13
finnsky commented@bronzehedwick thanks you for work. One thing here. Why --force push was used? It became hard to control changes
Comment #14
finnsky commentedI've added common library from https://www.drupal.org/project/drupal/issues/3028968 and added required optimisations.
Comment #15
bronzehedwickThanks for the review @m4olivei! Taking a look at those comments now.
@finnsky Sorry about that, I didn't know the etiquette here. I rebased the branch against 1.x, as there was some eslint related fixes there I wanted to integrate to properly lint the code here. I rebased to keep a linear history.
I also squashed my changes into the "Convert jQuery to vanilla Javascript - #3412125" commit, as I thought that was the correct protocol.
Should I make all my changes in separate commits?
Comment #16
m4oliveiHey @bronzehedwick. I'm not an expert on the etiquette, but from what I've gleaned, its generally best to avoid force push on a branch that you didn't open, and/or after others have joined in collaboratively on your MR, either via commiting themselves, or review comments. Once we have approval on a MR and the issue is marked as Reviewed & tested by the community (RTBC), the MR will be squashed & merged, so the commit history on the MR can be as verbose as it needs to be, usually the more the better, so I would say do make changes in separate commits. Some reviewers, myself included, sometimes prefer to walk through changes commit by commit.
Comment #17
bronzehedwickGot it, thanks @m4olivei! I'll follow that procedure in the future.
Comment #18
bronzehedwickComment #19
bronzehedwickComment #20
finnsky commented@bronzehedwick hello thank you for work!
Idk what to do now.
I wanted to move that filter back to separated file.
But seems you also did some changes there.
Probably you can split it to 2 commits? one for move and one for changes?
Also sadly navigationBlockSettingsSummary still not works for me.
Comment #21
finnsky commentedGonna review.
Comment #24
ckrinaI confirm
navigationBlockSettingsSummarydoesn't work, or the vertical tabs summary. To test it follow the videos @m4oliveiposted on his feedback in https://git.drupalcode.org/project/navigation/-/merge_requests/160#note_... innavigation-block.js.Comment #25
finnsky commentedWe can continue with current code with small piece of jquery for triggering and listening jquery events. Because jquery can listen vanilla events but vanila cannot catch jquery.
Next events need to be listened/triggered.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/form.js?...
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/form.js?...
Comment #26
bronzehedwickComment #27
m4oliveiThis is looking good to me! Nice work all!
RTBC for me, but I'll leave as Needs review for @finnsky to have another look.
Comment #28
finnsky commentedLooks good to me. Great work!
One thing here i would like to check deeper maybe:
I've checked table drag weights before and after patch. It works but behaves different. Not sure how critical it may be. As far as i understand that weights play some role in backend block positioning. Probably we need to test it with lot of blocks.
Before patch:
After:
Comment #29
finnsky commentedComment #30
finnsky commentedComment #31
finnsky commentedWe discussed in slack that it shouldn't affect backend. So let's move it to review. Thanks you team!
https://drupal.slack.com/archives/C7AB68LJV/p1710357432657529
Comment #33
ckrinaYass!! Merged! Great work and collaboration, team!