Merge lp:~yellow/launchpad/accordion-client-1 into lp:launchpad

Hi Gary,

Whew, done. There are 20-odd items that need addressing but most of them
are quite small. Bear in mind that some of these may be addressed in the
next branch, but I thought it better to make note of them now just in
case. I'm marking the branch as r=me as I think all of these can be
addressed fairly simply and I don't want to block the landing. If any
major revisions are made someone else can add their vote beside mine.

One thing that I did notice was that certain things (like the
trailing-comma-before-closing-brace problem, see [6]) should have been
picked up by our JS linter (which I think runs as part of `make lint.`
Could you re-run that for me and, if they're not being picked up, file a
bug?

[1]

236 + Y.on('domready', function() {
237 + module.setup_bug_subscriptions(
238 + {content_box: "#structural-subscription-content-box"})

This line needs a semicolon.

239 + });

And this need to be outdented by four spaces to avoid it looking like
it's closing the above JS Object.

[2]

236 + Y.on('domready', function() {
237 + module.setup_bug_subscriptions(
238 + {content_box: "#structural-subscription-content-box"})
239 + });

Same comments as [1].

[3]

395 + Bugs can be tagged to help the project categorise the bug report. You
396 + can filter your subscription based on one or more tags. If you can choose
397 + to <em>Match all tags</em> then your filter will only match bugs that
398 + have each of those tags you enter here.

Should the end of line 396 read "If you choose" rather than "If you can
choose..."?

[4]

476 + // The list may be undefined in some cases.
477 + return list && list.indexOf(target) != -1;

We should use isArray() explicitly here:

    return Y.Lang.isArray(list) && list.indexOf(target) != -1;

[5]

479 +namespace.list_contains = list_contains;

This seems to be a common pattern. Why not just make the function part
of the namespace in the first place:

namespace.list_contains = function(list, target) {
    ///...
};

(This is a common pattern elsewhere in the LP JS, though I don't know if
it's considered a standard). I'll not bother highlighting the other
cases; they're easy enough to find.

[6]

495 + statuses: [],
496 + };

The comma before the closing brace on an object literal causes problems
with some browsers (Opera and some versions of WebKit, IIRC); it should
be removed.

[7]

569 + on: {failure: overlay_error_handler.getFailureHandler()},

The trailing comma needs to die for the same reason as in [6].

[8]

575 +namespace._delete_filter = delete_filter

Needs a semicolon.

[9]

611 +namespace._add_bug_filter = add_bug_filter

So does this.

[10]

633 +function edit_subscription_handler(context, form_data) {

This function needs some documentation commentary.

[11]

649 +function create_overlay(content_box_id, overlay_id, submit_button,
650 + submit_callback) {

As does this one.

[12]

691 + alert('"Subscribe to bug mail" link not found.');

We shouldn't be using the alert() here any more. Y.fail would seem more
appropriate.

[13]...

> Whew, done. There are 20-odd items that need addressing but most of them
> are quite small.
[snip]
> One thing that I did notice was that certain things (like the
> trailing-comma-before-closing-brace problem, see [6]) should have been
> picked up by our JS linter (which I think runs as part of `make lint.`
> Could you re-run that for me and, if they're not being picked up, file a
> bug?

The JS linter was indeed not functioning. I filed bug 742619 which has
already provoked a pocketlint fix to be released.

In response to the broken linter I used Crockford's JSLint to guide me
in revision 12660.

A large number of missing semicolons and extra commas were
inserted/removed.

All == and != were changed do === and !==, respectively.

Most uses of string subscripts (foo['bar']) were changed to attribute
access (foo.bar).

Loops over arrays were changed from using the for-in syntax to instead
use traditional indexed for loops.

Added a few missing "var" declarations.

Instead of writing up an exhaustive justification for these changes I'll
let Mr. Crockford explain: http://javascript.crockford.com/code.html

I'll address each of the non-lint comments in a subsequent note.

I'm happy with the lint changes.

Revision 12662 has the below-mentioned changes.

> [5]
>
> 479 +namespace.list_contains = list_contains;
>
> This seems to be a common pattern. Why not just make the function part
> of the namespace in the first place:
>
> namespace.list_contains = function(list, target) {
> ///...
> };
>
> (This is a common pattern elsewhere in the LP JS, though I don't know if
> it's considered a standard). I'll not bother highlighting the other
> cases; they're easy enough to find.

This was done to decrease the identifier length. Otherwise instead of
just referencing "list_contains" in the body of the module, we'd have to
use "namespace.list_contains" throughout. The only reason the function
is needed outside of the namespace is for testing so the one-liner to
expose it to tests seems a reasonable trade-off.

> [10]
>
> 633 +function edit_subscription_handler(context, form_data) {
>
> This function needs some documentation commentary.

Done.

>
> [11]
>
> 649 +function create_overlay(content_box_id, overlay_id, submit_button,
> 650 + submit_callback) {

Done.

> [12]
>
> 691 + alert('"Subscribe to bug mail" link not found.');
>
> We shouldn't be using the alert() here any more. Y.fail would seem more
> appropriate.

Fixed.

> [13]
>
> 698 + e.preventDefault();
>
> There's a been a bug about the use of e.preventDefault() and
> e.stopPropagation() not working properly with some browsers (probably IE
> but ISTR some WebKit browsers had problems too). For safety we should
> use e.halt(), since in all the cases where one of {preventDefault,
> stopPropagation} fails, the other works fine, and e.halt() calls both.
> It's probably worth replacing all instances of preventDefault() in this
> JS with halt().

Fixed.

> [14]
>
> 708 +function clear_overlay(content_node) {
>
> This needs documentation.

Done.

> [15]
>
> 902 + * @param node The node to collapse.
> 903 + */
> 904 +function collapse_node(node, user_cfg) {
>
> user_cfg needs to be documented here, too.

Gary and I have been following the (self-imposed) guideline that only
functions meant for "public" consumption require the javadoc-like
documentation so I fixed the asymmetry in parameter documentation by
removing the docs for the other parameter.

> [16]
>
> 939 +/**
> 940 + * Expand the node and set its arrow to 'collapsed'
> 941 + * @param node The node to collapse.
> 942 + */
> 943 +function expand_node(node, user_cfg) {
>
> Same as [15].

Same fix as [15].

> [17]
>
> 977 +/**
> 978 + * Construct the overlay and populate it with the add/edit form.
> 979 + *
> 980 + * @method setup_overlay
> 981 + * @param {String} content_box_id Id of the element on the page where
> 982 + * the overlay is anchored.
> 983 + * @return {String} overlay_id Id of the constructed overlay element.
> 984 + */
> 985 +function setup_overlay(content_box_id, hide_recipient_picker) {
>
> There's no overlay_id parameter, so you can drop the documentation for
> it.

Same fix as [15].

> [18]
>
> 1242 + // if we want to allow editing of the recipient, do this
> instead
...