Code review comment for lp:~daker/ubuntu-html5-theme/fix.1232533

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

L250 use '==' for null checks (so that it accounts for 'undefined')
L255 (and after): add small checks at least to validate that the ul/lis are there, ... and should bail out if there are no <li>s (and subsequent calls should too)
L262 when expanded you dont set an active by default (as in 268)?
L270: why a global current index? it'll be shared between the option selector instances (might not matter since there might be only one but anyway)
L300 why the nodeType check?
L319-320: you can merge the 2 ifs (& same question about nodeType)
L318 might be interesting to have a var li = this.optionselector_ul_li[i] just to avoid all the property refs
L332-L340 make it a separate method/function (event raising),

review: Needs Fixing

« Back to merge proposal