launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

> #24
> Can you test if everything still works without the parent call in the
> initializer? If I recall correctly, YUI will stack the initializer methods for
> you properly so that they don't need to be manually called for classes
> extending other classes.

Done.

> #71
> Can you not just call: this.hide() vs the full parent method:
> Y.lazr.picker.Picker.prototype.hide.call(this);
> Is this not called from an instance perhaps? So it's meant to be called this
> way as if it was some sort of static method?

A quick test shows this doesn't work. This is defined in this.hide(), which may be part of the problem, and the object inheritance in this isn't entirely fixed. In truth, I want to not have new hide/show functions and move this extra stuff into a bindUI after('visibleChange') call, but that's happening in a subsequent branch.
 
> #82
> In _update_button_text: how about using an else statement after the if == team
> check. It avoids an accessor to the remove_person_text.

Done.

> #95
> Do we need the value: null? In the calling code if it's using Y.Lang.isValue()
> either undefined or null will return false. I guess this requires a quick
> check on the subscribers to see what they're doing with the value.

Not all subscribers for this are cleaned up yet--in fact, it's not entirely clear what all the possible subscribers *are*, and I'm not convinced they're adequately tested for me to just change and be comforted by tests passing. I've made a note of this for a follow up branch.

> #342
> The properties should just be outside the initializer method, they'd be on the
> same line as the initializer itself.

Ah, dig. Wasn't sure with the Y.Base.create where those went. Done.

> #457
> Any ATTR value that is passed into the initializer(cfg) will automatically get
> set to that ATTR as the current value. Lines that only check if it's set and
> if so, run a this.set('someattr', 'somevalue') can just be removed. I think
> this holds true for the next two attribute setters as well.

Done.

> #913
> Another case of wondering if you can just do this._defaultCancel(). If this is
> event based, can we check where the cancel event is fired from and see if it's
> calling the fire correctly? on the instance for example?

I've made a note again to look at this in followup, but this falls into the "I'm not yet confident to change here" category, so I've just preserved what's known to work.

> #1197
> If the default is null, you can just leave the value bit out.
> 
> I notice you doc these ATTRS, but not the ones in the PersonPicker. As I one
> day dream of running YUIDoc against our code to help generate nice docs for
> our JS modules, it'd be great if you could add it in. One of the good ones for
> ATTRS is the @default null which would indicate a default value of null in the
> docs.
> 
> If you're using Vim and snipmate, I've got a series of snippets for doing YUI
> doc blocks.
> http://paste.mitechie.com/show/701/

If you're alright with it, I'll tackle this in another follow up to clean up docs. This branch was slack time and expanded beyond that time allotment, so I'd like to get it landed and handle smaller bits in subsequent slack time.
 
> #1185
> You can shortcut this using the setAttrs(). It allows you to change that to
> something more like
> 
>     this.get('host').setAttrs({
>         'selected_value_metadata': result.metadata,
>         'selected_value', result.value
>     });

> #1468
> Looks like you killed a line on accident there.

The peril of holding shift while moving down in your code. :-P
-- 
https://code.launchpad.net/~jcsackett/launchpad/cleanup-pickers-with-base-create/+merge/110921
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.