Code review comment for lp:~wallyworld/launchpad/new-team-picker-simple-form

Ian, thanks for the work. In light of the recent discussions of our JS being a bit complicated and difficult to reuse, I've gotten very picky in this review. I apologize up front, but I hope that by simplifying and working on standardizing some we can make life a bit easier and point out more reusable bits as we come across code with names that match up.

The overall summary is that, while you're creating a team form specific class, that naming convention doesn't need to carry over through all of the code inside that class. It's simpler to drop it. It then starts to point out common method conventions, like render, errorHandler, etc.

Another is that there's a lot of use of Y.bind() when it's not necessary. It makes the code a bit less readable as it's carrying a lot of params for Y.bind() to function. I've written out one of the methods below to show how it could be done without any of the Y.bind() calls.

#221
    why not just stick the TEAM_CREATED and CANCEL_TEAM directly on the ns back after line 214?

#232
    In YUI there are some usual naming conventions. There's things like contentBox, etc. In 3.5 there's a standard 'container' used for things like this new_team_form. Could this be updated to be 'container' in this object?

    In a similar way can the _new_team_template just be a generic _get_template? You're in an object called CreateTeamForm so the new team bit is known and understood.

#233
    Along the same lines, we don't really need to prefix things as team_form_error_handler, but merely error_handler or maybe form_error_handler if required.

#237
    Since you've created the namespace for lp.app.picker.team and this is a form in that namespace, I'd suggest not creating a new namespace, but working off the current one. ns.form = {};
    You'd not need to store the namespace on the object instance since it's not really an instance variable. It's a namespace and global to all YUI code in this block.
    Then the #238 would just be callbacks = ns.form.callbacks;

#283
    I'd prefer for the event to just get a callback and that the event handler didn't do work for the widget. I'd move the stuff in on_success to CreateTeamForm._handle_success/_handle_failure and have the logic enclosed in them. Here it's difficult to find and test these handlers because they're defined within the _load_new_team_form method and doing too much.

    The on_success method might look something more like:
        success: function (id, response) {
            Y.Array.each(ns.form.callbacks, function (cb) {
                 cb._handle_success.call(cb, responseText);
            }
        }

    You'd not need to pass the callback method in #247, and the new _handle_success method would be able to bind events/etc. You'd only be pushing instance onto the callback stack.

        callbacks.push(this);

#314
    /render_new_team_form/render

#352
s/SubmitSpinner/Spinner

#402
    Setting a node = to form_data.id isn't clicking for me. Should it be the id of that node or is it meant to be the node itself?

#407
    None of those calls in your callback require the scope of the local instance. You can just call the methods. You can use the typical JS trick of storing that = this; at the top of the method to aid in getting access to the methods themselves.

So I'd write the _save_new_team method something like this: https://pastebin.canonical.com/68867/

#606
    Why do you wrap simulate and fire the event that way vs getting the node and calling node.simulate()? That's what the module node-event-simulate allows.

#676
    Please wrap the field.name in quotes.

#679
   So this turns into form_buttons.one('button:nth-child(1)').simulate('click');

#683
    /jaon/json

#691
    typo in event_publishd