Remove cruft from JavaScript code

follow up of http://drupal.org/node/444344 ... :)

This seems good. It's simple formatting changes e.t.c as you say!

Let's wait for the bot to come along then we can get it in.

This looks most appropriate and these standards should be documented in the Drupal coding standards.

This looks RTBC to me, except for the use of parseInt(___, 10). Is there any reason why we're explicitly telling parseInt to use the decimal numeric system? It defaults to decimal. Here it is without the 10, because it defaults to 10.......

Rob's patch is good - I read through it and there are only code style changes, no actual code changes.

Ah, great patch! :)

-  this.popup = document.createElement('div');
-  this.popup.id = 'autocomplete';
+  this.popup = $('<div id="autocomplete"></div>')[0];

...
-  var ul = document.createElement('ul');
+  var ul = $('<ul></ul>');
...
etc.

I actually find the old code much more legible than the new code. But is this considered a standard jQuery way to do things? If so, I'm fine with letting this slide, but that first one is definitely a 'wtf' and needs a comment explaining what it's doing. But if there isn't a 'standard' that says "write these things this way" my preference is the more legible one even though it's less 'clever.'

         .after($('<div class="fieldset-wrapper"></div>')
-        .append(fieldset.children(':not(legend):not(.action)')))
+          .append(fieldset.children(':not(legend):not(.action)')))
         .addClass('collapse-processed');

That looks like a mistake. Or is it to imply that .append is being appended to .after? If so, can we add a comment so the next 'helpful' person doesn't 'fix' this?

+  $(this.element).before($('<div class="error"></div>').html(string)).hide();

Let's get a comment above that sucker too. Wth? :)

-          window.location = uri+'&op=finished';
+          window.location = settings.batch.uri + '&op=finished';

That's quite different. Could we have confirmation from someone that batch API continues to work with this patch?

-    self.dragObject = new Object();
+    self.dragObject = {};
...
-          var values = new Array();
+          var values = [];

Another "wha-huh?" from me here. Again, would someone with general knowledge in JS/jQuery understand what this is, or is this something only crazy JS wizards like kkaefer understand? :) (these lines do have comments, in this case, but this construct is still very weird.) Again, if there isn't a 'standard' that says "write these things this way" my preference is the more legible one even though it's less 'clever.'

Those were the only ones that jumped out at me in glancing through.

Gotta say, $(this.element).before(error).hide() is plain beauty :p No errors on all browsers, everything seems to work normally too.

This clean up really helps the JavaScript. I've documented the typeof coding standard. We should go through the other changes made and document them in the JavaScript coding standards.

Webchick is kind of right in saying that the following looks kind of weird unless you look at it closely:

         .append(summary)
         .after($('<div class="fieldset-wrapper"></div>')
           .append(fieldset.children(':not(legend):not(.action)')))
         .addClass('collapse-processed');

Maybe this?

         .append(summary)
         .after($('<div class="fieldset-wrapper"></div>')
           .append(fieldset.children(':not(legend):not(.action)'))
         ).addClass('collapse-processed');

There's the patch with that change. Which ever method is chosen, I believe this is RTBC.

@RobLoach: Cool, yeah that's a bit more legible.

@kkaefer: Thanks for answering my questions and assuaging my worries/doubts. :)

Committed to HEAD. :) Thanks!

And at approximately 3:18pm Eastern on April 26th, the collective anguished moans of every JS patch author in the entire core queue could be heard around the world... Sorry, folks... but we discussed in #drupal-dev and figured there's another few hours left in the sprint to do clean-up work so sooner would be better than later. And hey, free chance to bump your patch in the queue and get some eyeballs on it! ;)

Also, let's get those coding standards documented so we never have to commit a patch like this again. ;)

This broke auto-completion. If I click on an auto-completion entry, e.g. an article tag, the contents of the text field is set to "undefined".

AFAICT the problem is this (though I haven't fully investigated the problem):

-    var li = document.createElement('li');
-    $(li)
-      .html('<div>'+ matches[key] +'</div>')
-      .mousedown(function () { ac.select(this); })
-      .mouseover(function () { ac.highlight(this); })
-      .mouseout(function () { ac.unhighlight(this); });
-    li.autocompleteValue = key;
-    $(ul).append(li);
+    $('<li></li>')
+      .html($('<div></div>').html(matches[key]))
+      .mousedown(function() { ac.select(this); })
+      .mouseover(function() { ac.highlight(this); })
+      .mouseout(function() { ac.unhighlight(this); })
+      .attr('autocompleteValue', key)
+      .appendTo(ul);

I don't think setting an autocompleteValue property on the object is equivalent to setting an autocompleteValue attribute.

we'll have to document that when we want to set an element's attribute, we use jquery's .attr() :)

tha_sun and stella pointed out to me that we do have JS coding standards, unbeknownst to me until just now. (I knew that there were people working on them, but I hadn't realized they'd gone through community review in the coding standards group, etc.). These standards are at http://drupal.org/node/172169.

This patch deviates from those in one important way: the space between the word "function" and the parentheses in anonymous functions. This is important because these are *not* functions, they are variables that contain a function.

So in addition to fixing autocomplete, let's back out that part of the changes. Should be a simple regex.

Having discussed this with tha_sun on IRC, we also think that we should be using .length rather than .size(). .length is faster because it's a simple property that jQuery always sets, whereas .size() adds a function call.

In addition, there is no current standard on the spacing style for .attr({....}). The patch changes these lines to be like:

myvar.attr({ foo: 'bar' });

Note the lack of a space between the parentheses and the curly brace, but the addition of a space between the curly braces and the attributes. We think the space between the { and the attributes should also be removed.

Cheers,
Stella

We're making changes in farbtastic.js here. Since it's a "third-party" plugin, those changes should be made in the Farbtastic project instead. Here's the patch without the Farbtastic change.

Tested the auto complete and it works now. RTBC.

I still see references to size() in there; let's get those switched to .length.

I defer to stella/sun/other JS folks on the spacing question. I do agree with kkaefer that it seems more legible with the space on either side, but it's also not something we do anywhere else in core. Is there an "official" jQuery coding style reference we can draw from to settle this?

Well, on the other hand, sun pointed out in IRC that we do currently have a critical bug here that needs fixing, and the size()->length and spacing questions are fairly minor things that could come after.

So I committed #23 to HEAD. Still leaving as needs work for #24.

deleted

It's not at all clear to me from browsing this what the "Needs Documentation" tag was added for. Can any of you participants elaborate? Or do the documentation?

Thanks!

I'm removing the Needs Doc tag, since it appears the standards are documented, based on the above discussion.

Awesome patch!

Quickly scanned the previous follow-ups as well as the committed patch -- parseInt() always needs to specify the second parameter "10" when assuming decimals. It is not the default. The default depends on the passed in value (first argument).

farbtastic.js is not affected, because it prefixes the passed in value with "0x".

Whitespace changes arn't really needed in this patch. ParseInt is a good one. RTBCings.

Why do we need to specify the 10? If the string begins with anything other than 0x or 0, the radix is automatically set to 10.

+++ misc/tabledrag.js	19 Sep 2009 03:27:40 -0000
@@ -524,11 +524,11 @@ Drupal.tableDrag.prototype.findDropTarge
+      var rowHeight = parseInt(row.offsetHeight, 10) / 2;

Although one can assume that in cases like this, offsetHeight is provided by the DOM, it is still commonly known as JavaScript best practice -- the script assumes a decimal value to work with, so the rowHeight can be used as decimal value for something else. Therefore, passing 10 means extra clarity.

+++ misc/tabledrag.js	19 Sep 2009 03:27:40 -0000
@@ -697,7 +697,7 @@ Drupal.tableDrag.prototype.updateField =
           // Assume a numeric input field.
-          var weight = parseInt($(targetClass, siblings[0]).val()) || 0;
+          var weight = parseInt($(targetClass, siblings[0]).val(), 10) || 0;
           $(targetClass, siblings).each(function () {
             this.value = weight;
             weight++;

Here, even the comment mentions that it assumes something and we try to perform calculations on an assumed decimal value. :)

This review is powered by Dreditor.

Fair enough. Committed to CVS HEAD. Thanks.