Code review comment for lp:~rvb/launchpad/bug-869221-ui

Revision history for this message
Raphaƫl Badin (rvb) wrote :

> I can't claim it's better but I thought you might like to see another
> way to use call chaining in showError:
>
> this.fieldNode
> .append(Y.Node.create('<div />')
> .addClass('message')
> .set("text", error))
> .ancestor('tr')
> .addClass('error');
>
> To my eyes the above makes the structure of what's happening a little
> clearer; not that it was particularly obscure to start with. The
> structural subscription JS uses this pattern to good effect.

I agree, it's more compact this way.

> Line 233 of the diff is indented by one space too many. Oh, and you
> don't really need the "check" variable.
>
> check_and_submit: function() {
> if (this.validate()) {
> this.submit();
> }
> }

Fixed.

> The join in testValidateFails surprised me. Is there a reason string
> concatenation won't work there?
>
> Assert.areEqual(
> "The distroseries has no architectures selected to "
> + "build architecture independent binaries.",
> this.widget.fieldNode.one('.message').get('text'));

Force of habit I guess ;) I've changed that.

Thanks for the review.

« Back to merge proposal