yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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
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#newcode176
> 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#newcode184
> 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#newcode1230
> 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#newcode391
> 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]

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.