launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Needs Fixing code*

Thanks for this. I really love to see this noscript stuff die in a fire. A few comments and info to look into. I'm going to mark as needs fixing mainly based on our irc conversation and the alt text note below.

- wgrant brought up that a div in span tag (line #24) isn't valid. This appears to 'run deep' though in the layout and I'm worried that the activator message box bits will break if it's not a div. Can you investigate? Can we just make it another span and css that up to be a display: block to make it quick/easy to update? This will require checking the css and the js for relying on the div bit in selectors and such for both classes yui3-activator-message-box and yui3-activator-hidden

- Should we be adding alt text to the <a links? Not sure if screen readers will do title if alt not there, etc. In doing a quick google, it seems many by default will not read title, but alt. Title is handy for hover in browsers though. I see comments that adding both with the same informationis bad though. The idea is that alt says what something *is* while title says where it'll go if clicked? So for the edit links, it might be an alt says 'edit button' while the title text would be 'edit object xxx'. 

- Discussed in irc the implications of having the <a tags all invisible-link, in particular for non-js users and non-graphical users.

- #239 the link goes out to egg? 
- #407 if the unseen css class isn't needed is the div needed at all?

-- 
https://code.launchpad.net/~sinzui/launchpad/progressive-enhancement-ftw/+merge/103733
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.