Review: Approve code
This looks ok, I wonder if the visible issue is that you didn't have a var in
the variable definition.
- Line #45 of the diff should have been var visible = ...
Please try that out and see if it fixes the issue you saw. If not I'll go ahead
and approve this change, but if we don't need to query the node for classes/etc
that'd be better.
- Line #87 there's a space change, was that intentional?
- var banner_node = Y.one(".global-notification") I always kind of hate the
.one() with a class because you could have multiple nodes with that class. Are
we sure this shouldn't be an id? How does this cascade down into the extending
classes. If you're sure it's ok, carry on, but want to make sure it's brought
up. This is in tests though so perhaps it should just be changed to pull from
the new banner_id property.
--
https://code.launchpad.net/~jcsackett/launchpad/banner-quick-fix/+merge/105500
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
