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/