launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Approve code

Thank you for this improvement. I have a few questions, but they wont block you because you have put this behind a feature flag (and this is an incremental branch.)

lines 121 and 122: <buttons> must have text for so users with screen readers can know what they do. Oh. I see the script sets the text later? Why? The buttons will only ever have one value, so you could inline the text in the string.

Why do you need a native DOM node. I see this pattern starting about line 190?
     Y.Node.getDOMNode(node.one('[id=field.name]')).value;
I expected
     node.one('[id=field.name]').get('value');

On 237 I see an image being loaded when the sprite is already loaded and the padding does not match the image:
    'style="background-image: url(/@@/person); ',
could be
    class="sprite person"

And in lines 247, maybe this should be
    class="sprite remove"

^ In both the preceding cases there is a class, but it probably duplicates sprite rules. Can we just use sprites?  That is to say, this is no longer lazr.js. I see you did exactly as I expect for a new team.
-- 
https://code.launchpad.net/~wallyworld/launchpad/new-team-picker/+merge/111346
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.