Code review comment for lp:~benji/juju-gui/add-rel-improvements-3

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode28
app/views/environment.js:28: this.addRelationDragStart.call(this, box,
context);
why do you need to use the 'call' method and pass 'this'?
You could simply call 'this.addRelationDragStart(box, context)', no?

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode133
app/views/environment.js:133: self.rectMousemove.call(node.getDOMNode(),
d, self);
why to you set the scope to getDOMNode and pass self ('this') as
parameter? You could pass the domNode as parameter instead...

self.rectMousemove(node.getDOMNode(), d);

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode176
app/views/environment.js:176: self.rectMousemove.call(node.getDOMNode(),
d, self);
It seems we have mousemove more than once in this file. You could
extract it to a common function in this closure.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode209
app/views/environment.js:209: self.backgroundClicked.call(self);
You dont need to use 'call'. You can do...

self.backgroundClicked();

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode446
app/views/environment.js:446: rectMousemove: function(d, self) {
According the the rest of the code, the scope here is domNode. If you
dont change the scope you can do (not tested... :) )...

// *************************
self.rectMousemove(node.getDOMNode());

rectMousemove: function(domNode) {
   var mouse = d3.mouse(domNode);
   d3.event.x = mouse[0];
   d3.event.y = mouse[1];
   this.addRelationDrag(this.get('addRelationStart_service'), domNode);
},
// *************************

btw, you dont use the 'd' parameter.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1231
app/views/environment.js:1231: possible_relations = Y.Array.map(
The indentation in this block is really strange. Maybe you could just
declare the variable and then assign the value to it in another line.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1529
app/views/environment.js:1529: view.startRelation.call(view, service);
It seems you like to use the 'call' function. :)

https://codereview.appspot.com/6821088/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/6821088/diff/1/undocumented#oldcode116
undocumented:116: app/views/environment.js click
On 2012/11/07 15:10:41, gary.poster wrote:
> The People of the Future thank you.

Hahahaha!

https://codereview.appspot.com/6821088/