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

Revision history for this message
Benji York (benji) wrote :

This branch looks good.

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.

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();
        }
    }

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'));

review: Approve (code)

« Back to merge proposal