Refactor the JavaScript for the widget to use the once library

Created on 22 May 2024, 7 months ago
Updated 20 September 2024, 3 months ago

Problem/Motivation

I noticed the JS for the widget isn't using the once() library. It's also using both the JS behaviors API combined with the jQuery document ready API which isn't really correct.

Since it registers a JS behavior, Drupal will re-invoke the code every time thee DOM loads initially, when BigPipe loads data, or when any other Drupal AJAX event takes place on the page. In my initial testing on a sample site, the behavior was executed 5 times, leading to all the event handlers being attached to DOM elements multiple times unnecessarily. This could lead to hard to debug issues in the future. It may even be a contributing factor to the problems encountered in πŸ› Office_hours Action links always point to frontpage on uncollapsed paragraphs Active

Proposed resolution

It should be refactored to rely on the Drupal behaviors API only, and combined with the once() library to ensure that event handlers are only attached once to the DOM elements. Without this, the handlers are attached multiple times unnecessarily.

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Needs review

Version

1.0

Component

Code - widget

Created by

πŸ‡ΊπŸ‡ΈUnited States bkosborne New Jersey, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @bkosborne
  • Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.1 & MySQL 5.7
    last update 7 months ago
    Waiting for branch to pass
  • Status changed to Needs review 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bkosborne New Jersey, USA

    Ready for review.

  • Pipeline finished with Failed
    7 months ago
    Total: 145s
    #179530
  • πŸ‡³πŸ‡±Netherlands johnv

    Thanks, I was aware that there was some technical debt/errors in the code.
    I will need to process the patch bit by bit, to check what has changed, and learn from it.

    • johnv β†’ committed cd667b05 on 8.x-1.x
      Issue #3449164 by bkosborne: Refactor the JavaScript - fixStriping()
      
  • πŸ‡³πŸ‡±Netherlands johnv
  • πŸ‡³πŸ‡±Netherlands johnv

    When code is reviewed, there are comments about unnamed functions, so I will keep the function name in attach: function doUpdateElement(context) {

    • johnv β†’ committed 2ba7dde1 on 8.x-1.x
      Issue #3449164 by bkosborne: Refactor the JavaScript - move functions...
    • johnv β†’ committed 46846d04 on 8.x-1.x
      Issue #3449164 by bkosborne: Refactor the JavaScript - limit scope of JS...
    • johnv β†’ committed 2d6b190d on 8.x-1.x
      Issue #3449164 by bkosborne: Refactor the JavaScript - add once library
      
  • Status changed to Needs work 7 months ago
  • πŸ‡³πŸ‡±Netherlands johnv

    I went through all changes, and understood and copied all but one:
    The following patch is still pending, since adding it breaks the following scenario (testing with dev-version):
    - enable Exceptions in field settings, using the correct Widget.
    - Push the 'Add exception' button. Then an element is rebuilt by a Ajax call. after that, the JS-links are broken.
    - You may still add an exception and save the node.
    - Now reopen the node, and see that without pressing 'Add exception', the exception's JS-links are fine, but broken after pressing the button.

    Thanks for your work!

    diff --git a/js/office_hours.js b/js/office_hours.js
    index b25721f..07a596a 100644
    --- a/js/office_hours.js
    +++ b/js/office_hours.js
    @@ -203,7 +203,7 @@
     
       Drupal.behaviors.office_hours = {
         attach: function doUpdateElement(context) {
    -     $(document).ready(function prepareElements() {
    +     $(once('office-hours', '.field--type-office-hours', context)).each(function prepareElements() {
             // Attach a function to each JS link and initialize if needed.
             // N.B.: using * wildcard, since initially, no suffix is added,
             // but after 'Add exception'button, a suffix is added to the ID.
    
    • johnv β†’ committed f53b2a0a on 8.x-1.x
      Issue #3449164 by bkosborne: Refactor the JavaScript - js/...
  • πŸ‡³πŸ‡±Netherlands johnv

    The past commit contains similar changes to js/office_hours_status_update.js.
    Please check that file.

  • πŸ‡³πŸ‡±Netherlands johnv

    I added the 'data-drupal-selector' to the code in πŸ“Œ Use not-randomized drupal-data-selector, not #id Fixed and I credited you for that.

  • First commit to issue fork.
  • Pipeline finished with Success
    3 months ago
    Total: 139s
    #288348
  • πŸ‡§πŸ‡ͺBelgium andreasderijcke Antwerpen / Gent

    After merge of the changes made up until 1.19, there was not much left but swapping out the $(document).ready with the once().

    That, also seems to fix πŸ› Additional slot row disappears after value change when used in nested paragraphs Needs work . I tested with a field in

    1. Tabs
    2. Details
    3. A Paragraph
    4. A Paragraph in Details
  • Status changed to Needs review 3 months ago
  • πŸ‡§πŸ‡ͺBelgium andreasderijcke Antwerpen / Gent
  • Status changed to Needs work 3 months ago
  • πŸ‡§πŸ‡ͺBelgium andreasderijcke Antwerpen / Gent

    I forgot the exception case, that is still broken.

  • Assigned to andreasderijcke
  • Status changed to Active 3 months ago
  • πŸ‡§πŸ‡ͺBelgium andreasderijcke Antwerpen / Gent
  • Pipeline finished with Success
    3 months ago
    Total: 143s
    #288495
  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • πŸ‡§πŸ‡ͺBelgium andreasderijcke Antwerpen / Gent

    I've updated the script to

    1. Apply all the initialisation and event handler binding per slot/row instead of the entire table at once. This makes sure new rows added by Ajax get the required processing. The 'once' at field level was blocking this.
    2. The striping fix was kept at field level, but it just occurs to me that I didn't check if that needs to be rerun after an exception is added, or when using the list widget.

    Also
    if ($nextTr.is(':hidden')) {
    didn't work anymore for some reason, so I changed it to
    if ($nextTr.hasClass('js-office-hours-hide')) {
    which seems to have the same effect as far as I can tell.

  • πŸ‡§πŸ‡ͺBelgium andreasderijcke Antwerpen / Gent

    I've updated the script to

    1. Apply all the initialisation and event handler binding per slot/row instead of the entire table at once. This makes sure new rows added by Ajax get the required processing. The 'once' at field level was blocking this.
    2. The striping fix was kept at field level, but it just occurs to me that I didn't check if that needs to be rerun after an exception is added, or when using the list widget.

    Also
    if ($nextTr.is(':hidden')) {
    didn't work anymore for some reason, so I changed it to
    if ($nextTr.hasClass('js-office-hours-hide')) {
    which seems to have the same effect as far as I can tell.

    • johnv β†’ committed befbc3fe on 8.x-1.x
      Issue #3449164 by andreasderijcke, bkosborne, max.valetov: Refactor the...
  • πŸ‡³πŸ‡±Netherlands johnv

    Thanks a lot. The fix seems to address the problem. Committed.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024