launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Approve code

Oooh, pretty lots of red with a little green back in. Approving, but there's a few small tweaks to things I'd like to suggest. 


- Please make sure to link the test files to the build directory vs the local one. Basically we're ensuring that you're testing what's in the build which the combo loader will serve. In this way part of the test is verify that the files are getting 'built' correctly. (mustache, banner) 

  Also looks like you moved the test_information_type_choice.html links?

- Another case of please move the #maincontent node creation to the setup() so that teardown isn't actually setting up the next test.

- The test in test_information_type_choice.js I'm a bit leery of firing global show/hide events. I can't think of anything that'd be listening, but there seems to be a big potential of triggering something listening out there unknowingly. What about just namespacing those a bit more to be testBannerShow or something?

- In that same test file you removed the dep on lp.app.privacy, but didn't add in the new banner? It looks like it's still needed in the shim code.

- In line 678 of the diff you don't use the getPrivacyBanner? Is there any reason for the change in usage there? 


-- 
https://code.launchpad.net/~jcsackett/launchpad/use-banner-to-cleanup-privacy/+merge/105731
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.