← Back to team overview

launchpad-reviewers team mailing list archive

  1. launchpad-reviewers team
  2. Mailing list archive
  3. Message #07704

Re: [Merge] lp:~jcsackett/launchpad/banner-class into lp:launchpad

  Thread Previous • Date Previous • • Thread Next 
Review: Approve code

Thanks so much for the updates. I think this is much more in line with the "YUI" way for sure.

- Please dbl check your dependencies (requires: ) and make sure anything you include into your test html file is required somewhere so that we don't break with the combo loader. Widget is missing from the banner module.

- Small lint nitpick, the spacing of lines 22/23

- I tend to prefer to have the private underscored methods at the top of the file and the others below. 

- Small performance tweaks: Your animation methods accept a node. You've already searched the page for the node so you can just pass the node instance and save some extra queries to the DOM. For example, in line 43 of the diff you get the body node. Then in line 55 you end up re'finding the node because you pass it a selector vs the node object you have in hand. This is also true of the global_notification node.

- You add a css class on the <body> element, I think the teardown method should make sure that's removed as well.

These are small tweaks though so approving with some cleanup suggestions. Thanks for bearing through this!
-- 
https://code.launchpad.net/~jcsackett/launchpad/banner-class/+merge/104984
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

References

  Thread Previous • Date Previous • • Thread Next