Code review comment for lp:~wallyworld/launchpad/new-team-picker

Revision history for this message
Ian Booth (wallyworld) wrote :

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

« Back to merge proposal