← Back to team overview

yellow team mailing list archive

Re: Add tests for panzoom. (issue 7001047)

 

Land with changes.

Hey Brad.  Thank you.  Looks good.

Actually, unrelated to your work, but new branches are broken right now
because of the new d3.  Could you change packages.json to specify d3 in
this way?

"d3": "2.10.x"

I agree with Matt that it would be nice if you tried to make a sentence
with "it."  I also agree that I'm kinda low on caring.

I wish you didn't have to redefine Y, but I'm guessing you did it for a
reason.  Please at least try to change the async: false approach that
Kapil suggests for the "before" Y, and then delete the "beforeEach"
version, and see if that works.  If not, land it.

Thanks

Gary


https://codereview.appspot.com/7001047/diff/1/test/test_panzoom.js
File test/test_panzoom.js (right):

https://codereview.appspot.com/7001047/diff/1/test/test_panzoom.js#newcode23
test/test_panzoom.js:23: Y = YUI(GlobalConfig).use(['node',
Why can't you just use the Y from before?  This is a pattern I have not
seen before in our tests.

https://codereview.appspot.com/7001047/

-- 
https://code.launchpad.net/~bac/juju-gui/1083935/+merge/141135
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bac/juju-gui/1083935 into lp:juju-gui.


References