yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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 extra credit if you also explain in the comment why we can't
do some kind of bubbling for this event handling, rather than
duplicating the same basic handler for multiple elements.  Maybe it's
just a standard d3 thing that is obvious to anyone who uses it, in which
case nm.

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

Your description is the truth but not the whole truth, right?  If you
can describe what else is going on, please do.  If not, please at least
clarify that other mysterious things are going on also, so a comment
reader will know to look a bit more closely.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode209
app/views/environment.js:209: self.backgroundClicked.call(self);
Please try "self.backgroundClicked();"

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode447
app/views/environment.js:447: var mouse = d3.mouse(this);
Commenting that "this" is not what it appears to be would be
appreciated. (Yuck :-/ but I accept your explanation that this is
standard in this file for some reason, albeit with concern.)

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode451
app/views/environment.js:451: .call(self,
self.get('addRelationStart_service'), this);
This ought to work as

"self.addRelationDrag(self.get('addRelationStart_service'), this);"

Please give it a try.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1074
app/views/environment.js:1074: // Create a pending drag-line behind
services.
Is it really "behind" now?  I think it is "in front of" because of the
change to append.  Am I right?

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1203
app/views/environment.js:1203: * @method backgroundClicked
Might as well add a "@return {undefined}" in there, as suggested by
yuidoc docs.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1214
app/views/environment.js:1214: * @method startRelation
@param and @return

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1226
app/views/environment.js:1226: service, getServiceEndpoints(), db),
I'm guessing that's the best indentation that our linter allows?  would
be nice if it could come in by 2 or 4 spaces.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1230
app/views/environment.js:1230: */
This has broken indentation.

Can we simply use "//" comments for this?  Automation will be able to
indent/dedent them better.  I'd really prefer for "/* */" comments to
only be used for yuidoc comments.  Tools have a much harder time
interpreting what to do within them.

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1232
app/views/environment.js:1232: Y.Array.flatten(Y.Object.values(
can we indent this 2 or 4 spaces?

https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode1529
app/views/environment.js:1529: view.startRelation.call(view, service);
Try view.startRelation(service) please

https://codereview.appspot.com/6821088/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/6821088/diff/1/test/test_environment_view.js#newcode391
test/test_environment_view.js:391: view.startRelation(service);
Nice approach to bypassing the need to simulate clicks.  That said,
before we propagate this approach, we should verify that d3, which Kapil
reports is tested well, does not have an alternate approach that might
be superior in some way (at the very lease because other d3 devs might
be familiar with it).

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
The People of the Future thank you.

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

-- 
https://code.launchpad.net/~benji/juju-gui/add-rel-improvements-3/+merge/133104
Your team Juju GUI Hackers is subscribed to branch lp:juju-gui.