Merge lp:~benji/juju-gui/add-rel-improvements-3 into lp:juju-gui/experimental

Proposed by Benji York
Status: Merged
Merged at revision: 233
Proposed branch: lp:~benji/juju-gui/add-rel-improvements-3
Merge into: lp:juju-gui/experimental
Diff against target: 553 lines (+216/-93)
5 files modified
README (+1/-1)
app/views/environment.js (+151/-91)
docs/style-guide.rst (+38/-0)
test/test_environment_view.js (+26/-0)
undocumented (+0/-1)
To merge this branch: bzr merge lp:~benji/juju-gui/add-rel-improvements-3
Reviewer Review Type Date Requested Status
Gary Poster (community) Approve
Review via email: mp+133104@code.launchpad.net

Description of the change

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :
Download full text (7.1 KiB)

Hey Benji. Thank you for this. It is a nice improvement that worked
well when I tried it. Tests passed and merge with trunk was also fine
for me.

I have a bunch of fairly trivial comments & requests. If you agree,
please just make the changes and treat this as an automatic approval.

Gary

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#newcode1
app/views/environment.js:1: 'use strict';
Please add the module yuidoc comment.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode10
app/views/environment.js:10: var EnvironmentView =
Y.Base.create('EnvironmentView', Y.View,
Please add the class yuidoc comment.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode23
app/views/environment.js:23: /** The user clicked on the "Build
Relation" menu item. */
We had a good conversation in a hangout about my mild concerns about the
fact that this is required because of yuidoc-linter even though yuidoc
ignored it; and about the fact that you are using a /** TEXT */ syntax
when I thought we were standardizing on a multiline syntax:
/**
  * TEXT
  */

I still have those concerns, and would feel slightly happier if you
changed to always use the multiline spelling (particularly given the use
of "/* */" rather than "//") but I am OK with letting you document and
advocate for your positions, in your role as
Bringer-of-YUIDoc-to-the-Masses.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode28
app/views/environment.js:28: this.addRelationDragStart.call(this, box,
context);
Could you check to see if "this.addRelationDragStart(box, context);"
will work? I think it will.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode82
app/views/environment.js:82: }
not entirely idle thought, but one that is not really directly pertinent
to your branch: I wonder how a service is supposed to make a
relationship to itself, which IIUC is possible.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode128
app/views/environment.js:128: */
As mentioned yesterday, please add @method, @param and @return tags with
the appropriate bits and bobs.

Oh, wait: this is an event handler, so yuidoc will not document it.

:-( I find this a bit hard to catch.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode133
app/views/environment.js:133: self.rectMousemove.call(node.getDOMNode(),
d, self);
So, where other uses of "call" in this file seem pointless, this one
actually seems to have a point...and it seems like a really odd pattern
to me. As we discussed yesterday, you are following an existing
pattern, so I'm not asking for any changes from you, but could you find
out from Matt or Ben why this pattern exists, and arrange for the
information to be shared with the rest of us, either from them or you?

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode171
app/views/environment.js:171: */
Add @method, @param, @return, please. ^D^D^D Nevermind, event handler.

Unnecessary ext...

Read more...

Revision history for this message
Gary Poster (gary) :
review: Approve
Revision history for this message
Thiago Veronezi (tveronezi) wrote :

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/

223. By Benji York

add YUIDoc info to the style guide

224. By Benji York

refactorings prompted by review

Revision history for this message
Gary Poster (gary) wrote :
Download full text (7.7 KiB)

On 2012/11/08 17:25:06, benji wrote:
> I have addressed everything other than one comment by Gary about the
tests in
> which I couldn’t figure out what he wanted.

Thank you!

I'd be happy to re-review if you wanted, but it is not up in Rietveld so
I won't unless you ask. I'll reply to a few comments here in-line
below, though.

...

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode23
> app/views/environment.js:23: /** The user clicked on the "Build
Relation" menu
> item. */
> On 2012/11/07 15:10:41, gary.poster wrote:
> > We had a good conversation in a hangout about my mild concerns about
the fact
> > that this is required because of yuidoc-linter even though yuidoc
ignored it;
> > and about the fact that you are using a /** TEXT */ syntax when I
thought we
> > were standardizing on a multiline syntax:
> > /**
> > * TEXT
> > */
> >
> > I still have those concerns, and would feel slightly happier if you
changed to
> > always use the multiline spelling (particularly given the use of "/*
*/"
> rather
> > than "//") but I am OK with letting you document and advocate for
your
> > positions, in your role as Bringer-of-YUIDoc-to-the-Masses.

> The multi-line format seems to overemphasize the importance of the
comment, so I
> have left these. However, I have updated the style guide to include a
(first
> draft) policy that explains the situations in which you would use one
or the
> other.

Cool, sounds good. Thank you.

...

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode128
> app/views/environment.js:128: */
> On 2012/11/07 15:10:41, gary.poster wrote:
> > As mentioned yesterday, please add @method, @param and @return tags
with the
> > appropriate bits and bobs.
> >
> > Oh, wait: this is an event handler, so yuidoc will not document it.
> >
> > :-( I find this a bit hard to catch.

> Yep, the plethora of event handlers makes this code hard to grok. I
am not sure
> what we should do instead. As an improvement I refactored the code so
there is
> a single view method that gets called when any of the mousemove events
are
> triggered. That method now has full docs.

Huh, cool. Sounds good, thank you.

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

> This was preexisting code (that got moved around). It is indeed
insane. I
> refactored this extensively and it now doesn't do this.

:-) Cool!

Ben told be that ".call" has the advantage of naturally supporting
chaining. However, in my own experiments I could not get this to work
so I may have misunderstood.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode171
> app/views/environment.js:171: */
> On 2012/11/07 15:10:41, gary.poster wrote:
> > Add @method, @param, @return, please. ^D^D^D Nevermind, event
han...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'README'
--- README 2012-11-05 19:35:15 +0000
+++ README 2012-11-08 17:28:23 +0000
@@ -105,7 +105,7 @@
105105
106 $ make yuidoc106 $ make yuidoc
107107
108The documentation for yuidoc markup is at108The documentation for YUIDoc markup is at
109http://yui.github.com/yuidoc/syntax/ .109http://yui.github.com/yuidoc/syntax/ .
110110
111Running lint111Running lint
112112
=== modified file 'app/views/environment.js'
--- app/views/environment.js 2012-11-07 12:32:38 +0000
+++ app/views/environment.js 2012-11-08 17:28:23 +0000
@@ -1,4 +1,9 @@
1'use strict';1'use strict';
2/**
3 * Provides the main app class.
4 *
5 * @module views
6 */
27
3YUI.add('juju-view-environment', function(Y) {8YUI.add('juju-view-environment', function(Y) {
49
@@ -7,6 +12,12 @@
7 Templates = views.Templates,12 Templates = views.Templates,
8 models = Y.namespace('juju.models');13 models = Y.namespace('juju.models');
914
15 /**
16 * Display an environment.
17 *
18 * @class environment
19 * @namespace views
20 */
10 var EnvironmentView = Y.Base.create('EnvironmentView', Y.View,21 var EnvironmentView = Y.Base.create('EnvironmentView', Y.View,
11 [views.JujuBaseView], {22 [views.JujuBaseView], {
12 events: {23 events: {
@@ -20,17 +31,19 @@
20 },31 },
21 // Menu/Controls32 // Menu/Controls
22 '.add-relation': {33 '.add-relation': {
34 /** The user clicked on the "Build Relation" menu item. */
23 click: function() {35 click: function() {
24 var box = this.get('active_service'),36 var box = this.get('active_service'),
25 service = this.serviceForBox(box),37 service = this.serviceForBox(box),
26 context = this.get('active_context');38 context = this.get('active_context');
39 this.addRelationDragStart(box, context);
27 this.service_click_actions40 this.service_click_actions
28 .toggleControlPanel(box, this, context);41 .toggleControlPanel(box, this, context);
29 this.service_click_actions42 this.service_click_actions.addRelationStart(box, this, context);
30 .addRelationStart(box, this, context);
31 }43 }
32 },44 },
33 '.view-service': {45 '.view-service': {
46 /** The user clicked on the "View" menu item. */
34 click: function() {47 click: function() {
35 // Get the service element48 // Get the service element
36 var box = this.get('active_service'),49 var box = this.get('active_service'),
@@ -42,6 +55,7 @@
42 }55 }
43 },56 },
44 '.destroy-service': {57 '.destroy-service': {
58 /** The user clicked on the "Destroy" menu item. */
45 click: function() {59 click: function() {
46 // Get the service element60 // Get the service element
47 var box = this.get('active_service'),61 var box = this.get('active_service'),
@@ -72,6 +86,12 @@
72 if (!d.containsPoint(mouse_coords, self.zoom)) {86 if (!d.containsPoint(mouse_coords, self.zoom)) {
73 return;87 return;
74 }88 }
89
90 // Do not fire if we're on the same service.
91 if (d === self.get('addRelationStart_service')) {
92 return;
93 }
94
75 self.set('potential_drop_point_service', d);95 self.set('potential_drop_point_service', d);
76 self.set('potential_drop_point_rect', rect);96 self.set('potential_drop_point_rect', rect);
77 self.addSVGClass(rect, 'hover');97 self.addSVGClass(rect, 'hover');
@@ -112,7 +132,8 @@
112 self.dragline.attr('class',132 self.dragline.attr('class',
113 'relation pending-relation dragline dragging');133 'relation pending-relation dragline dragging');
114 }134 }
115 }135 },
136 mousemove: 'mousemove'
116 },137 },
117 '.sub-rel-block': {138 '.sub-rel-block': {
118 mouseenter: function(d, self) {139 mouseenter: function(d, self) {
@@ -143,16 +164,30 @@
143164
144 // Relation Related165 // Relation Related
145 '.rel-label': {166 '.rel-label': {
146 click: 'relationClick'167 /** The user clicked on the relation label. */
168 click: 'relationClick',
169 mousemove: 'mousemove'
147 },170 },
148171
149 // Canvas related
150 '#canvas rect:first-child': {172 '#canvas rect:first-child': {
173 /**
174 * If the user clicks on the background we cancel any active add
175 * relation.
176 */
151 click: function(d, self) {177 click: function(d, self) {
152 var container = self.get('container');178 var container = self.get('container');
153 container.all('.environment-menu.active').removeClass('active');179 container.all('.environment-menu.active').removeClass('active');
154 self.service_click_actions.toggleControlPanel(null, self);180 self.service_click_actions.toggleControlPanel(null, self);
155 self.cancelRelationBuild();181 self.cancelRelationBuild();
182 },
183 mousemove: 'mousemove'
184 },
185 '.dragline': {
186 /** The user clicked while the dragline was active. */
187 click: function(d, self) {
188 // It was technically the dragline that was clicked, but the
189 // intent was to click on the background, so...
190 self.backgroundClicked();
156 }191 }
157 }192 }
158 },193 },
@@ -168,28 +203,12 @@
168 return;203 return;
169 }204 }
170205
171 // set a flag on the view that we're building a relation
172 self.buildingRelation = true;
173
174 // Sometimes mouseover is fired after the mousedown, so ensure206 // Sometimes mouseover is fired after the mousedown, so ensure
175 // we have the correct event in d3.event for d3.mouse().207 // we have the correct event in d3.event for d3.mouse().
176 d3.event = e;208 d3.event = e;
177209
178 // Flash an indicator around the center of the service block.
179 var center = d.getCenter();
180 self.vis.append('circle')
181 .attr('cx', center[0])
182 .attr('cy', center[1])
183 .attr('r', 100)
184 .attr('class', 'mouse-down-indicator')
185 .transition()
186 .duration(750)
187 .ease('bounce')
188 .attr('r', 0)
189 .remove();
190
191 // Start the process of adding a relation210 // Start the process of adding a relation
192 self.addRelationDragStart.call(self, d, this);211 self.addRelationDragStart(d, this);
193 }, [d, evt], false);212 }, [d, evt], false);
194 },213 },
195 'mouseup.addrel': function(d, self) {214 'mouseup.addrel': function(d, self) {
@@ -400,6 +419,27 @@
400 self.removeRelationConfirm(d, this, self);419 self.removeRelationConfirm(d, this, self);
401 },420 },
402421
422 /**
423 * If the mouse moves and we are adding a relation, then the dragline
424 * needs to be updated.
425 *
426 * @method mousemove
427 * @param {object} d Unused.
428 * @param {object} self The environment view itself.
429 * @return {undefined} Side effects only.
430 */
431 mousemove: function(d, self) {
432 if (self.clickAddRelation) {
433 var container = self.get('container'),
434 node = container.one('#canvas rect:first-child').getDOMNode(),
435 mouse = d3.mouse(node);
436 d3.event.x = mouse[0];
437 d3.event.y = mouse[1];
438 self.addRelationDrag
439 .call(self, self.get('addRelationStart_service'), node);
440 }
441 },
442
403 /*443 /*
404 * Sync view models with current db.models.444 * Sync view models with current db.models.
405 */445 */
@@ -461,7 +501,7 @@
461 })501 })
462 .on('drag', function(d, i) {502 .on('drag', function(d, i) {
463 if (self.buildingRelation) {503 if (self.buildingRelation) {
464 self.addRelationDrag.call(self, d, this);504 self.addRelationDrag(d, this);
465 } else {505 } else {
466 if (self.longClickTimer) {506 if (self.longClickTimer) {
467 self.longClickTimer.cancel();507 self.longClickTimer.cancel();
@@ -483,7 +523,7 @@
483 })523 })
484 .on('dragend', function(d, i) {524 .on('dragend', function(d, i) {
485 if (self.buildingRelation) {525 if (self.buildingRelation) {
486 self.addRelationDragEnd.call(self, d, this);526 self.addRelationDragEnd();
487 }527 }
488 });528 });
489529
@@ -1020,23 +1060,26 @@
1020 },1060 },
10211061
1022 addRelationDragStart: function(d, context) {1062 addRelationDragStart: function(d, context) {
1023 // Create a pending drag-line behind services.1063 // Create a pending drag-line.
1024 var dragline = this.vis.insert('line', '.service')1064 var dragline = this.vis.append('line')
1025 .attr('class', 'relation pending-relation dragline dragging'),1065 .attr('class', 'relation pending-relation dragline dragging'),
1026 self = this;1066 self = this;
10271067
1028 // Start the line in the middle of the service.1068 // Start the line between the cursor and the nearest connector
1029 var point = d.getCenter();1069 // point on the service.
1030 dragline.attr('x1', point[0])1070 var mouse = d3.mouse(Y.one('svg').getDOMNode());
1031 .attr('y1', point[1])1071 self.cursorBox = views.BoundingBox();
1032 .attr('x2', point[0])1072 self.cursorBox.pos = {x: mouse[0], y: mouse[1], w: 0, h: 0};
1033 .attr('y2', point[1]);1073 var point = self.cursorBox.getConnectorPair(d);
1074 dragline.attr('x1', point[0][0])
1075 .attr('y1', point[0][1])
1076 .attr('x2', point[1][0])
1077 .attr('y2', point[1][1]);
1034 self.dragline = dragline;1078 self.dragline = dragline;
1035 self.cursorBox = views.BoundingBox();
10361079
1037 // Start the add-relation process.1080 // Start the add-relation process.
1038 self.service_click_actions1081 self.service_click_actions
1039 .addRelationStart(d, self, context);1082 .addRelationStart(d, self, context);
1040 },1083 },
10411084
1042 addRelationDrag: function(d, context) {1085 addRelationDrag: function(d, context) {
@@ -1057,7 +1100,7 @@
1057 }1100 }
1058 },1101 },
10591102
1060 addRelationDragEnd: function(d, context) {1103 addRelationDragEnd: function() {
1061 // Get the line, the endpoint service, and the target <rect>.1104 // Get the line, the endpoint service, and the target <rect>.
1062 var self = this;1105 var self = this;
1063 var rect = self.get('potential_drop_point_rect');1106 var rect = self.get('potential_drop_point_rect');
@@ -1133,11 +1176,80 @@
1133 this.dragline.remove();1176 this.dragline.remove();
1134 this.dragline = null;1177 this.dragline = null;
1135 }1178 }
1179 this.clickAddRelation = null;
1136 this.set('currentServiceClickAction', 'toggleControlPanel');1180 this.set('currentServiceClickAction', 'toggleControlPanel');
1181 this.buildingRelation = false;
1137 this.show(this.vis.selectAll('.service'))1182 this.show(this.vis.selectAll('.service'))
1138 .classed('selectable-service', false);1183 .classed('selectable-service', false);
1139 },1184 },
11401185
1186 /**
1187 * The user clicked on the environment view background.
1188 *
1189 * If we are in the middle of adding a relation, cancel the relation
1190 * adding.
1191 *
1192 * @method backgroundClicked
1193 * @return {undefined} Side effects only.
1194 */
1195 backgroundClicked: function() {
1196 if (this.clickAddRelation) {
1197 this.cancelRelationBuild();
1198 }
1199 },
1200
1201 /**
1202 * An "add relation" action has been initiated by the user.
1203 *
1204 * @method startRelation
1205 * @param {object} service The service that is the source of the
1206 * relation.
1207 * @return {undefined} Side effects only.
1208 */
1209 startRelation: function(service) {
1210 // Set flags on the view that indicate we are building a relation.
1211 this.buildingRelation = true;
1212 this.clickAddRelation = true;
1213
1214 this.show(this.vis.selectAll('.service'));
1215
1216 var db = this.get('db'),
1217 getServiceEndpoints = this.get('getServiceEndpoints'),
1218 endpoints = models.getEndpoints(
1219 service, getServiceEndpoints(), db),
1220 // Transform endpoints into a list of relatable services (to the
1221 // service).
1222 possible_relations = Y.Array.map(
1223 Y.Array.flatten(Y.Object.values(endpoints)),
1224 function(ep) {return ep.service;}),
1225 invalidRelationTargets = {};
1226
1227 // Iterate services and invert the possibles list.
1228 db.services.each(function(s) {
1229 if (Y.Array.indexOf(possible_relations,
1230 s.get('id')) === -1) {
1231 invalidRelationTargets[s.get('id')] = true;
1232 }
1233 });
1234
1235 // Fade elements to which we can't relate.
1236 // Rather than two loops this marks
1237 // all services as selectable and then
1238 // removes the invalid ones.
1239 this.fade(this.vis.selectAll('.service')
1240 .classed('selectable-service', true)
1241 .filter(function(d) {
1242 return (d.id in invalidRelationTargets &&
1243 d.id !== service.id);
1244 }))
1245 .classed('selectable-service', false);
1246
1247 // Store possible endpoints.
1248 this.set('addRelationStart_possibleEndpoints', endpoints);
1249 // Set click action.
1250 this.set('currentServiceClickAction', 'ambiguousAddRelationCheck');
1251 },
1252
11411253
1142 /*1254 /*
1143 * Zoom in event handler.1255 * Zoom in event handler.
@@ -1403,51 +1515,10 @@
1403 * flow.1515 * flow.
1404 */1516 */
1405 addRelationStart: function(m, view, context) {1517 addRelationStart: function(m, view, context) {
1406 view.show(view.vis.selectAll('.service'));1518 var service = view.serviceForBox(m);
14071519 view.startRelation(service);
1408 var db = view.get('db'),
1409 getServiceEndpoints = view.get('getServiceEndpoints'),
1410 service = view.serviceForBox(m),
1411 endpoints = models.getEndpoints(
1412 service, getServiceEndpoints(), db),
1413
1414 /* Transform endpoints into a list of
1415 * relatable services (to the service in m)
1416 */
1417 possible_relations = Y.Array.map(
1418 Y.Array.flatten(Y.Object.values(
1419 endpoints)),
1420 function(ep) {return ep.service;}),
1421 invalidRelationTargets = {};
1422
1423 // Iterate services and invert the possibles list.
1424 db.services.each(function(s) {
1425 if (Y.Array.indexOf(possible_relations,
1426 s.get('id')) === -1) {
1427 invalidRelationTargets[s.get('id')] = true;
1428 }
1429 });
1430
1431 // Fade elements to which we can't relate.
1432 // Rather than two loops this marks
1433 // all services as selecable and then
1434 // removes the invalid ones
1435 view.fade(view.vis.selectAll('.service')
1436 .classed('selectable-service', true)
1437 .filter(function(d) {
1438 return (d.id in invalidRelationTargets &&
1439 d.id !== m.id);
1440 }))
1441 .classed('selectable-service', false);
1442
1443
1444
1445 // Store start service in attrs.1520 // Store start service in attrs.
1446 view.set('addRelationStart_service', m);1521 view.set('addRelationStart_service', m);
1447 // Store possible endpoints.
1448 view.set('addRelationStart_possibleEndpoints', endpoints);
1449 // Set click action.
1450 view.set('currentServiceClickAction', 'ambiguousAddRelationCheck');
1451 },1522 },
14521523
1453 /*1524 /*
@@ -1479,19 +1550,8 @@
1479 return a[0].name + a[1].name < b[0].name + b[1].name;1550 return a[0].name + a[1].name < b[0].name + b[1].name;
1480 });1551 });
14811552
1482 // Create a pending line if it doesn't exist, as in the case of1553 // Stop rubberbanding on mousemove.
1483 // clicking to add relation.1554 view.clickAddRelation = null;
1484 if (!view.dragline) {
1485 var dragline = view.vis.insert('line', '.service')
1486 .attr('class', 'relation pending-relation dragline'),
1487 points = m.getConnectorPair(
1488 view.get('addRelationStart_service'));
1489 dragline.attr('x1', points[0][0])
1490 .attr('y1', points[0][1])
1491 .attr('x2', points[1][0])
1492 .attr('y2', points[1][1]);
1493 view.dragline = dragline;
1494 }
14951555
1496 // Display menu with available endpoints.1556 // Display menu with available endpoints.
1497 var menu = container.one('#ambiguous-relation-menu');1557 var menu = container.one('#ambiguous-relation-menu');
14981558
=== modified file 'docs/style-guide.rst'
--- docs/style-guide.rst 2012-10-04 17:07:28 +0000
+++ docs/style-guide.rst 2012-11-08 17:28:23 +0000
@@ -5,6 +5,7 @@
5This guide is an attempt to describe a code style that both works well with the5This guide is an attempt to describe a code style that both works well with the
6JavaScript beautifier and helps developers avoid pitfalls.6JavaScript beautifier and helps developers avoid pitfalls.
77
8
8Indentation9Indentation
9===========10===========
1011
@@ -13,18 +14,21 @@
13beautifier ("make beautify"). The lowest (least leading whitespace) acceptable14beautifier ("make beautify"). The lowest (least leading whitespace) acceptable
14indention will be applied.15indention will be applied.
1516
17
16For loops18For loops
17=========19=========
1820
19Unless you are counting something, for loops (and for-in loops) are a trap.21Unless you are counting something, for loops (and for-in loops) are a trap.
20Use Y.Object.each instead.22Use Y.Object.each instead.
2123
24
22Whitespace25Whitespace
23==========26==========
2427
25No trailing whitespace on lines or at the end of the file (i.e., the file28No trailing whitespace on lines or at the end of the file (i.e., the file
26should end with a non-blank line).29should end with a non-blank line).
2730
31
28Object literal formatting32Object literal formatting
29=========================33=========================
3034
@@ -61,6 +65,7 @@
61 type: 'int'}},65 type: 'int'}},
62 errors = utils.validate(values, schema);66 errors = utils.validate(values, schema);
6367
68
64Chaining method calls69Chaining method calls
65=====================70=====================
6671
@@ -118,6 +123,7 @@
118 .bing()123 .bing()
119 .zing());124 .zing());
120125
126
121Creating HTML127Creating HTML
122=============128=============
123129
@@ -148,3 +154,35 @@
148154
149It is important that the indentation of the calls communicates the structure of155It is important that the indentation of the calls communicates the structure of
150the resulting DOM tree. Compare and contrast the above examples.156the resulting DOM tree. Compare and contrast the above examples.
157
158
159Comments
160========
161
162We use YUIDoc to document the applications internals. YUIDoc comments
163start with "/**" and end with "*/". The Makefile includes a simple
164linter that enforces YUIDoc comments for each function in the
165application.
166
167This simple linting sometimes means that functions that we might not
168otherwise document require documentation. If a one-line comment is
169sufficient in those situations, a comment of this form may be used:
170
171 /** Handle errors */
172 error_callback: function(err) {
173 ...
174 }
175
176Most functions (or methods) will call for normal, multi-line YUIDoc
177comments like this:
178
179 /**
180 * Frob the thingy.
181 *
182 * @method frob
183 * @param {object} type How the thingy should be frobbed.
184 * @return {undefined} Side-effects only, eturns nothing.
185 */
186
187Full documentation for the various YUIDoc directives is at
188http://yui.github.com/yuidoc/syntax/ .
151189
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js 2012-11-01 13:21:53 +0000
+++ test/test_environment_view.js 2012-11-08 17:28:23 +0000
@@ -340,6 +340,9 @@
340 container.all('.selectable-service')340 container.all('.selectable-service')
341 .size()341 .size()
342 .should.equal(2);342 .should.equal(2);
343 container.all('.dragline')
344 .size()
345 .should.equal(1);
343 service.next().simulate('click');346 service.next().simulate('click');
344 container.all('.selectable-service').size()347 container.all('.selectable-service').size()
345 .should.equal(0);348 .should.equal(0);
@@ -370,6 +373,28 @@
370 view.get('rmrelation_dialog').hide();373 view.get('rmrelation_dialog').hide();
371 });374 });
372375
376 it('should stop creating a relation if the background is clicked',
377 function() {
378 var db = new models.Database(),
379 endpoint_map = {'service-1': {requires: [], provides: []}},
380 view = new views.environment(
381 { container: container,
382 db: db,
383 env: env,
384 getServiceEndpoints: function() {return endpoint_map;}}),
385 service = new models.Service({ id: 'service-1'});
386
387 db.services.add([service]);
388 view.render();
389
390 // If the user has clicked on the "Add Relation" menu item...
391 view.startRelation(service);
392 assert.isTrue(view.buildingRelation);
393 // ...clicking on the background causes the relation drag to stop.
394 view.backgroundClicked();
395 assert.isFalse(view.buildingRelation);
396 });
397
373 // TODO: This will be fully testable once we have specification on the398 // TODO: This will be fully testable once we have specification on the
374 // list view itself. Skipped until then.399 // list view itself. Skipped until then.
375 it.skip('must be able to switch between graph and list views',400 it.skip('must be able to switch between graph and list views',
@@ -390,6 +415,7 @@
390 }415 }
391 );416 );
392 });417 });
418
393 describe('view model support infrastructure', function() {419 describe('view model support infrastructure', function() {
394 var Y, views, models;420 var Y, views, models;
395421
396422
=== modified file 'undocumented'
--- undocumented 2012-11-06 02:04:22 +0000
+++ undocumented 2012-11-08 17:28:23 +0000
@@ -110,7 +110,6 @@
110app/views/environment.js attachSceneEvents110app/views/environment.js attachSceneEvents
111app/views/environment.js buildScene111app/views/environment.js buildScene
112app/views/environment.js cancelRelationBuild112app/views/environment.js cancelRelationBuild
113app/views/environment.js click
114app/views/environment.js destroyService113app/views/environment.js destroyService
115app/views/environment.js destroyServiceConfirm114app/views/environment.js destroyServiceConfirm
116app/views/environment.js detachSceneEvents115app/views/environment.js detachSceneEvents

Subscribers

People subscribed via source and target branches