Code review comment for lp:~wallyworld/launchpad/sharing-details-delete-966641

Revision history for this message
Curtis Hovey (sinzui) wrote :

I see the SharingDetailsTable.initializer lost its bugs and branches setters, yet the tests show everything works. What this unused data? I think the bugs and branches in the cache were the real data source, not the config.

Line 355 repeats the common mistake of creating a sprite without allocating space for it. Elements with sprites must contain content, add a nbsp;
    <span class="sprite bug-{{bug_importance}}">&nbsp;</span>',

Line 529 has bogus markup "<br><br>" never use two <br />, because more than one means your markup and css is broken. Most browser strip all but the first one when rendering sibling blocks because they know css is the correct way to handle the situation. the <br /> must state it is empty as well. I can see the style attr is used with a class. This markup is just broken. I am certain it came from somewhere else in Lp.

This is a small semantic issue I share with Julian, "should" implies we are uncertain about how the code we wrote operates. "Should" on lines 729 and 999 adds a Heisenberg/Wittgenstein state of uncertainty, when I am very certain the module "must" be loaded for the test to complete. Maybe
 "Could not locate the "

review: Approve (code)

« Back to merge proposal