We can remove the -processed hacks now that once is in core.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

litwol’s picture

Status: Active » Needs review
FileSize
9.67 KB

Things to note:
* I've converted js/*.js files to use .once() to the best of my understanding, except stylizer.js
* stylizer.js is relatively more complicated than other files, so i am leaving it to be tackled last once i receive confirmation that changes i've introduced to other files are on the correct track.
* Be thorough when reviewing & testing this patch as I have negligible experience with ctools js [framework?].
* this patch is a little longer than it should have been due to realigned code blocks after removing/introducing new scope ({}) blocks.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/js/dependent.jsundefined
@@ -227,7 +227,7 @@
-        .trigger('change');
+      .trigger('change');

From the patch context, its not clear why this was changed.

+++ b/js/dropdown.jsundefined
@@ -24,15 +24,14 @@
           var $dropdown = $(this);
           var open = false;
           var hovering = false;
           var timerID = 0;

This is now indented too far

+++ b/js/modal.jsundefined
@@ -192,83 +192,70 @@
+        $(this).click(Drupal.CTools.Modal.clickAjaxLink);

Maybe this is a good section for var $this = $(this); since its used 4 times

+++ b/js/modal.jsundefined
@@ -192,83 +192,70 @@
+        $(this).click(Drupal.CTools.Modal.clickAjaxLink);

Possibly here too? $(this) is used twice, as well as $(button) which is the same.

+++ b/js/modal.jsundefined
@@ -192,83 +192,70 @@
+        element_settings.url = $(this).attr('action');

And 3 times here as well.

+++ b/js/modal.jsundefined
@@ -192,83 +192,70 @@
-      $('.ctools-close-modal', context).once('ctools-close-modal-processed', function () {
-        $(this).click(function() {
+      $('.ctools-close-modal', context).once('ctools-close-modal')
+        .click(function() {
           Drupal.CTools.Modal.dismiss();
           return false;
         });

Nice clean-up!

litwol’s picture

Status: Needs work » Needs review
FileSize
16.14 KB

* Stylizer.js still largely unchanged. still needs to be addressed with .once() rewrite.
* integrated above feedback

merlinofchaos’s picture

-      $("select.ctools-master-dependent:not(.ctools-processed)")
-        .addClass('ctools-processed')
+      $("select.ctools-master-dependent")
+        .once('ctools')

Should probably use ctools-dependent rather than just ctools -- too much risk of accidental collision. It should probably never have been 'ctools-processed' in the first place.

litwol’s picture

Status: Needs review » Needs work
FileSize
16.15 KB

Minor patch update, integrating feedback from #4.

Next i will upload converted stylizer.js.

litwol’s picture

Assigned: Unassigned » litwol
Status: Needs work » Needs review

as per conversation on irc, stylizer.js is not to be addressed as part of this patch. there is other work that needs to be done on stylizer.js before (if ever) converting it to .once(). more info here: #1275992: Removing anonymous functions in stylizer.js

tim.plunkett’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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