← Back to team overview

launchpad-reviewers team mailing list archive

Re: [Merge] lp:~jcsackett/launchpad/kill-lazr-3 into lp:launchpad

 

> A general note, there's no .html files in here so I just want to make sure
> that the tests are all ok. They're including the same .js files I suppose and
> shouldn't have inline JS that needs use() updates. For a sanity check, just
> want to make sure the same number of tests run in develop/trunk as this branch
> to make sure no tests failed to fire for any reason.

I'll double check, but a grep for lazr only turned up a reference to a lazr/lazr.js (which was irrelevant, since the new ui/ui.js file was also included). I deleted that line.

> #321
> The lazr.editor requirement went away. Was it not needed?
Correct; it's not the only place where there are unneeded modules in the required statement, but I decided that I would tackle those another day.
 
> #466
> I worry that you've created the namespace lp.ui, but you went from Y.lazr to
> Y.lp.ui.overlay as the extend. Is Y.lp.ui.overlay something that exists
> already as a namespace in prev code? I see that FormOverlay is still under
> just Y.lp.ui and not in the additional subspace of .overlay.
You're right, it should just be lp.ui.PrettyOverlay, not lp.ui.overlay. I've corrected that.
 
> #807
> Since you changed the NAME I think there might be CSS implications with that.
> I just want to make sure we QA those overlays to make sure that there's not
> any CSS in that particular change that will break while the rest of the CSS is
> updated. There tests might pass while the CSS might be borked a bit.

That's a test file; the subclass only exists in the test, which is why I was comfortable changing it.
-- 
https://code.launchpad.net/~jcsackett/launchpad/kill-lazr-3/+merge/123648
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.


References