launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Needs Fixing

Thank you so much for cleaning this up. I know it's a big chunk, but really think the LoC going down as it gets cleaned up and more readable is a great sign.

I'm going to mark needs fixing just because I want to chat on the event driven methods and their use of the explicit calling of the parent class before it lands. It could just be a case of 'that is the way it has to be' to avoid needing to rework a bunch of other things, but want to check.

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

Example Fiddle: http://jsfiddle.net/NdTHr/1/

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

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

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

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

Sample fiddle: http://jsfiddle.net/jSpWL/1/

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

Sample fiddle: http://jsfiddle.net/X3ERp/1/

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

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

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




-- 
https://code.launchpad.net/~jcsackett/launchpad/cleanup-pickers-with-base-create/+merge/110921
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.