launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

On Fri, Apr 4, 2014 at 12:29 AM, William Grant <me@xxxxxxxxxxxxxxxxxx>wrote:

> Review: Needs Fixing code
>
> 7       /**
> 8       - * Reset the widget to a blank state.
> 9       + * Reset the widget to a blank state and fires
> 10      + * 'CodeReviewComment.updated' event.
> 11      *
>
> Wouldn't it be better to fire the event in insert_comment_HTML, between
> the appendChild and green_flash? The event name also doesn't make a whole
> lot of sense.
>
>
Right, firing 'CodeReviewComment.appended' from inser_comment_HTML.


> 139     + setter: function(navigator) {
> 140     + var self = this;
> 141     + var container = self.get('srcNode').one('.diff-navigator')
> 142     + container.append(navigator)
> 143     + navigator.on('change', function() {
> 144     + self._showPreviewDiff(this.get('value'), false);
> 145     + });
>
> I don't think the self/this bit does what you intend. self is only useful
> inside the callback, but you use self outside and this inside.
>

Once self is defined I get tempted to use it everywhere, but I agree it
causes more confusing than benefits. Reviewed 'self' use on the whole
module and preserved it only when it's needed.


> 248     + if (!Y.Lang.isValue(navigator)) {
> 249     + // DiffNav was not properly initialized.
> 250     + return
> 251     +
>
> Missing semicolon.


Added.

[]
-- 
Celso Providelo
celso.providelo@xxxxxxxxxxxxx

https://code.launchpad.net/~cprov/launchpad/ic-ui-stab-two/+merge/213950
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.