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

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

I think the sign here was that you were sending the callback stack both the object and a method on that object to be used by the callback bits. Only one should do and you can treat it more like a list, etc.

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

The that = this is just standard JS. I think it goes back to JS the Good Parts book, but I'd have to go look it up. Y almost never use Y.bind and it's purely selfish. I find the that=this format easier to read, less to type, etc.

Really the one time I do use Y.bind is when the scope isn't this, but some other arbitrary object. It could be anything in there and sometimes happens I want to do some event magic with the scope of some other object I know about (combining Views and such).

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

It might be. Perhaps it was before the node-event-simulate module.