Hi Rick

Thanks for the very thorough review. I do appreciate it.

> 
> 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. 
>

[1] I lot of the naming issues (well almost all) are because I
originally had this code inside the picker widget and moved it out into
a new, separate widget. When the code lived in it's original home, the
variable and method names made more sense.


> 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?
> 

I was following existing convention used in most of the lazr-js
codebase. I am guessing, but believe the reason for doing it this wy is
so that the event names can be used internally without having to
dereference off the namespace. I can see the reasoning for doing it this
way.

> #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. 
>

Yes, will fix. See [1].


> #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.
>

Yes, will fix. See [1].


> #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;
> 

Yes, will fix. See [1].

> #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);
> 


In general I do try and keep event handlers lean and mean and move out
the work to methods for testability. I didn't think the callbacks were
too complicated in this case. Nevertheless, I'll fix them up.

> #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?
>

The id of the form node I believe but am not 100%. I followed what was
done in FormOverlay. I'll check.


> #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.
> 

Till this branch I have been doing the that = this thing in my work but
thought that the approach was a bit kludgy since my reading of the
Y.bind docs seemed to indicate that using bind and passing in 'this'
seemed more kosher. If you tell me it is definitely the preferred
approach to use that = this, I'll revert to that pattern. When I've been
doing it, I use self = this as that naming seems more logical to me.

> 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.
>

Most/all of our existing codebase does it that way. Perhaps it was due
to earlier limitations in YUI. Not sure.


> #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
> 
>