launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Approve code

IE 8 (and 7) does not support :not(). I think you need to write this like this:
    span.update-in-progress-message {}
    button.update-in-progress-message:after {}

I think the inline style on 402 is wrong. "style="max-width: 60em;" is only used when we know there is no side bar. sibling elements are 45ems. Why does this element allowed to extend beyond their length?

I see this pattern:
     +            '    class="sprite remove action-icon remove-duplicate-bug">',
  415+            '    Remove</a>',
which puts leading white-space between the start of the anchor and the text. Even though the links are action-icon, parts of the underline can appear in some browsers, and quite likely for users that increase the text size. Maybe this is safer
     +            '    class="sprite remove action-icon remove-duplicate-bug">',
  415+                 'Remove</a>',

TAL process its instructions in a specific order, define, then condition. This also means that define is always called on the element. Even though you wrote the condition first, the define was already executed:
  570+             tal:condition="context/bug/duplicateof|nothing"
  571+             tal:define="duplicateof context/bug/duplicateof">
Since we know the order we could write this:
  570+             tal:define="duplicateof context/bug/duplicateof"
  571+             tal:condition="duplicateof|nothing">


-- 
https://code.launchpad.net/~wallyworld/launchpad/improve-dupe-bug-ui-227310/+merge/118738
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.