launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Approve

Yay for cleaned up JS. I love the final lines where removing of some setup overhead takes place.

#8 
I'd drop a couple of those blank lines. Maybe stick with the Python rule of 2 spaces between blocks?

#86 
can you hoist the that=this up to the top of the method. It tends to provide some visual hints as to what's going on below that can be easy to blow over when it's buries in between things.

#114
I'd skip the callback and just have the toggle_hidden be the callback. You can assign the current 'this' scope by adding it as the final param in .delegate. 

See http://yuilibrary.com/yui/docs/event/#extended-signature

Then toggle_hidden would just take the event passed usually and you'd have this being the Comment object and the event would have a currentTarget and target for the nodes clicked.

See http://yuilibrary.com/yui/docs/event/#basics

#23
I'd suggest making this into a ATTR with a valueFn so that it's only called once, and you don't repeat the selector in multiple places in the code.
-- 
https://code.launchpad.net/~jcsackett/launchpad/comment-list-delegates/+merge/116376
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.