Code review comment for lp:~edwin-grubbs/lazr-js/activator-ie-fixes

Revision history for this message
Māris Fogels (mars) wrote :

Hi Edwin,

These changes look good, r=mars. I'm surprised by how subtle the IE fixes are.

The one question I had was: do we need the tabIndex attribute to be readonly, or is just setting to '-1' sufficient? Did we just prevent anyone from ever passing their own tabIndex into the constructor of the Widget? Put another way, does this effectively lock up tabIndex with "final int tabIndex = -1"?

I have only a few suggestions for polish:

• lines 147,170: Use .one() instead of .query()
• In the comment on line 192, the explanation is excellent, but please also explain why you chose to use get('tabIndex') instead of getAttribute('tabIndex')
• 374: instead of a for-in loop, which can be error-prone, consider using Y.each()

Maris

review: Approve

« Back to merge proposal