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

CommentFileSizeAuthor
#5 ctools-utilize-once-1505432-v3.patch16.15 KBlitwol
#3 ctools-utilize-once-1505432-v2.patch16.14 KBlitwol
#1 ctools-utilize-once-1505432.patch9.67 KBlitwol

Comments

litwol’s picture

litwol commented
Status: Active » Needs review
StatusFileSize
new ctools-utilize-once-1505432.patch9.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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
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

litwol commented
Status: Needs work » Needs review
StatusFileSize
new ctools-utilize-once-1505432-v2.patch16.14 KB

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

merlinofchaos’s picture

merlinofchaos commented 
-      $("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

litwol commented
Status: Needs review » Needs work
StatusFileSize
new ctools-utilize-once-1505432-v3.patch16.15 KB

Minor patch update, integrating feedback from #4.

Next i will upload converted stylizer.js.

litwol’s picture

litwol commented
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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Fixed

Committed!
http://drupalcode.org/project/ctools.git/commit/5d7edd1
Thanks litwol.

Status: Fixed » Closed (fixed)

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