launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

> > #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. 
-- 
https://code.launchpad.net/~wallyworld/launchpad/new-team-picker-simple-form/+merge/111781
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.