Issue 6821088: Improve dragline behavior.
DescriptionImprove dragline behavior.
https://code.launchpad.net/~benji/juju-gui/add-rel-improvements-3/+merge/133104
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 55
MessagesTotal messages: 5
Please take a look.
Sign in to reply to this message.
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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... app/views/environment.js:209: self.backgroundClicked.call(self); Please try "self.backgroundClicked();" https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... app/views/environment.js:1214: * @method startRelation @param and @return https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... 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#newcod... 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#newcod... 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#newcod... 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#n... 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.
Sign in to reply to this message.
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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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!
Sign in to reply to this message.
I have addressed everything other than one comment by Gary about the tests in which I couldn’t figure out what he wanted. 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'; On 2012/11/07 15:10:41, gary.poster wrote: > Please add the module yuidoc comment. Done. 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, On 2012/11/07 15:10:41, gary.poster wrote: > Please add the class yuidoc comment. Done. 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. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode28 app/views/environment.js:28: this.addRelationDragStart.call(this, box, context); On 2012/11/07 15:10:41, gary.poster wrote: > Could you check to see if "this.addRelationDragStart(box, context);" will work? > I think it will. It does indeed work. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode28 app/views/environment.js:28: this.addRelationDragStart.call(this, box, context); On 2012/11/07 15:21:04, thiago wrote: > why do you need to use the 'call' method and pass 'this'? > You could simply call 'this.addRelationDragStart(box, context)', no? Right. That's fixed. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcode82 app/views/environment.js:82: } On 2012/11/07 15:10:41, gary.poster wrote: > 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. That is a good question. I tried a bit but I couldn't get the app to do it. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... 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. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... 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. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... 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 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. I don't understand event bubbling. However I did refactor this so there is only a single view method that handles these events and the three existing mousemove events call it. > Maybe it's just a standard d3 thing that is > obvious to anyone who uses it, in which case nm. I have no idea. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:176: self.rectMousemove.call(node.getDOMNode(), d, self); On 2012/11/07 15:21:04, thiago wrote: > It seems we have mousemove more than once in this file. You could extract it to > a common function in this closure. Good catch. I have refactored the code so there is instead only a single view method that all of these evens call. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:176: self.rectMousemove.call(node.getDOMNode(), d, self); On 2012/11/07 15:21:04, thiago wrote: > It seems we have mousemove more than once in this file. You could extract it to > a common function in this closure. Yep, fixed as per earlier comment. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:184: * relation. On 2012/11/07 15:10:41, gary.poster wrote: > Add @method, @param, @return please. ^D^D^D Nevermind, event handler. Refactored away, see above. > Your description is the truth but not the whole truth, right? It is all the truth that I am aware of. :) > 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. I don't know that there are other mysterious things are going on. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:209: self.backgroundClicked.call(self); Fixed. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:446: rectMousemove: function(d, self) { On 2012/11/07 15:21:04, thiago wrote: > According the the rest of the code, the scope here is domNode. If you dont > change the scope you can do (not tested... :) )... Yep, this got refactored considerably, somewhat along the lines you outlined. > btw, you dont use the 'd' parameter. Indeed. Unfortunately there is some sort of framework thing going on here that foists arguments upon functions. I documented the fact that the argument is unused. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:447: var mouse = d3.mouse(this); On 2012/11/07 15:10:41, gary.poster wrote: > 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.) The refactoring removed the local "this" craziness. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:451: .call(self, self.get('addRelationStart_service'), this); On 2012/11/07 15:10:41, gary.poster wrote: > This ought to work as > > "self.addRelationDrag(self.get('addRelationStart_service'), this);" Yep. I wonder why there was so much of this pattern in the code. I have found several other instances of FOO.method.call(FOO, ...) and fixed them. If we had an easy to use linting system, it would be nice to add a warning about such constructs. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1074: // Create a pending drag-line behind services. On 2012/11/07 15:10:41, gary.poster wrote: > Is it really "behind" now? I think it is "in front of" because of the change to > append. Am I right? Done. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1203: * @method backgroundClicked On 2012/11/07 15:10:41, gary.poster wrote: > Might as well add a "@return {undefined}" in there, as suggested by yuidoc docs. Done. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1214: * @method startRelation On 2012/11/07 15:10:41, gary.poster wrote: > @param and @return Done. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1226: service, getServiceEndpoints(), db), On 2012/11/07 15:10:41, gary.poster wrote: > 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. Fixed. This was bad when it was moved from another location. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1230: */ On 2012/11/07 15:10:41, gary.poster wrote: > 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. This comment was in this code when I moved it from elsewhere. I converted it to use leading // comments. I am a little surprised that tooling would have trouble with the C-style comments. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1231: possible_relations = Y.Array.map( On 2012/11/07 15:21:04, thiago wrote: > The indentation in this block is really strange. Agreed. I didn't notice how weird it was when I moved it from somewhere else. I have fixed it. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1232: Y.Array.flatten(Y.Object.values( On 2012/11/07 15:10:41, gary.poster wrote: > can we indent this 2 or 4 spaces? This was inherited from elsewhere, but the fix was easy. https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... app/views/environment.js:1529: view.startRelation.call(view, service); On 2012/11/07 15:21:04, thiago wrote: > It seems you like to use the 'call' function. :) The code I inherited likes the call method. I have fixed this and all other instances of this anti-pattern. 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#n... test/test_environment_view.js:391: view.startRelation(service); On 2012/11/07 15:10:41, gary.poster wrote: > 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). I can't discern what action you want me to take and the time frame in which you want me to take 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 On 2012/11/07 15:10:41, gary.poster wrote: > The People of the Future thank you. heh
Sign in to reply to this message.
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#newcod... > 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#newcod... > 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#newcod... > 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 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. > > I don't understand event bubbling. What I meant was classic DOM event bubbling: IOW, as you are familiar, if you have a div that holds a span that holds an image, when the user clicks on the image, ignoring lots of details, the click event first fires in the image, and then the span, and then the div. I'm guessing that the svg elements we care about have no idea of containment, and that's why we have to explicitly set all of these up. > However I did refactor this so there is only > a single view method that handles these events and the three existing mousemove > events call it. Cool, thanks. ... > https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... > app/views/environment.js:176: self.rectMousemove.call(node.getDOMNode(), d, > self); > On 2012/11/07 15:21:04, thiago wrote: > > It seems we have mousemove more than once in this file. You could extract it > to > > a common function in this closure. > > Good catch. I have refactored the code so there is instead only a single view > method that all of these evens call. Sounds great. > https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... > app/views/environment.js:184: * relation. > On 2012/11/07 15:10:41, gary.poster wrote: > > Add @method, @param, @return please. ^D^D^D Nevermind, event handler. > > Refactored away, see above. > > > Your description is the truth but not the whole truth, right? > > It is all the truth that I am aware of. :) > > > 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. > > I don't know that there are other mysterious things are going on. I was specifically looking at "self.service_click_actions.toggleControlPanel(null, self);" and thinking that it did something beyond what you mentioned. If I'm wrong that's fine. ... > https://codereview.appspot.com/6821088/diff/1/app/views/environment.js#newcod... > app/views/environment.js:1230: */ > On 2012/11/07 15:10:41, gary.poster wrote: > > 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. > > This comment was in this code when I moved it from elsewhere. I converted it to > use leading // comments. I am a little surprised that tooling would have > trouble with the C-style comments. The way I see it, with "/* */" comments *all* whitespace between the opening and closing is potentially meaningful to humans. It's a fence that reasonably means "hands off, this is for humans." With "//" comments, the whitespace to the left of the delimiter is not in human comment land. That's my understanding of why the beautifier works this way. > https://codereview.appspot.com/6821088/diff/1/test/test_environment_view.js#n... > test/test_environment_view.js:391: view.startRelation(service); > On 2012/11/07 15:10:41, gary.poster wrote: > > 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). > > I can't discern what action you want me to take and the time frame in which you > want me to take it. = Actions for Benji to Take (START RIGHT AWAY!!!!!!1!) = Step 1: Pat self on back. Good job with that test, self! Step 2: Land this branch. Step 3: Pat self on back. Good job with that branch, self! Step 4: Make sure there is a card in the retrospective lane about discussing and propagating what you did for Friday. Steps 5...N: Eat, Sleep, Profit, etc. Step N+1: Take a look at d3 tests and see if you can figure out if they tackle this kind of problem, and if so, what they do. Steps N+2...M: Eat, Sleep, Profit, etc. Step M+1: Report pertinent discoveries, successes and experiences to the group at the Friday retrospective tomorrow. Step M+2: Pat self on back. Good job with that report, self! [END ACTIONS]
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
