launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

>
> 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.
>
I can do that. The pattern I used mimicked the approach used in the 
picker validation forms where the button did have different text and so 
a generic form skeleton was created and the button text set later. But 
here it is fixed, you are right.

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

I did indeed try the approach you suggest but get('value') kept 
returning ''. I have no idea why. I'll try it again.

> 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.

That code was cut and pasted from elsewhere and put into a method for 
convenience. Yes, I should have noticed the old pattern and replaced 
with sprites.  I'll fix.



-- 
https://code.launchpad.net/~wallyworld/launchpad/new-team-picker/+merge/111346
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.