launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

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.