yellow team mailing list archive

[Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx>]

Thank you both for the review feedback. I've made the changes and will
propose again after getting these comments out.

The most significant change is the expanded handling around event
phases.


https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode40
app/assets/javascripts/d3-components.js:40: var Component =
Y.Base.create('Component', Y.Base, [], {
On 2012/11/08 13:33:35, thiago wrote:
> I like this code style. You could the same with the class above. I
mean...

> var Module = ...

The linter complained that I didn't combine the var decls into one set,
I prefer the other style as well. I'll see if I cna get lint to shut up.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode62
app/assets/javascripts/d3-components.js:62: //
this.after('containerChange', this.bind());
On 2012/11/08 13:33:35, thiago wrote:
> You could remove the commented code.

Done.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode70
app/assets/javascripts/d3-components.js:70: *
On 2012/11/08 18:49:46, matthew.scott wrote:
> Minor - Docs don't match method signature.

Done.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode85
app/assets/javascripts/d3-components.js:85: options = options || {};
On 2012/11/08 13:33:35, thiago wrote:
> I don't like setting a value to one function parameter.

I agree its questionable, questionable but common. Still, I can alter
it.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode93
app/assets/javascripts/d3-components.js:93: this.modules[module.name] =
module;
On 2012/11/08 13:33:35, thiago wrote:
> Funny indentation

fixjsstyle must have done this, fixed.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode95
app/assets/javascripts/d3-components.js:95: var modEvents =
module.events;
On 2012/11/08 13:33:35, thiago wrote:
> Usually we have only one var per closure. Maybe you can declare the
variables in
> the first line and then attribute values to it here.

Done.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode97
app/assets/javascripts/d3-components.js:97: this.bind(module.name);
On 2012/11/08 18:49:46, matthew.scott wrote:
> Curious about the necessity for not doing Y.clone(module.events);
here.
> Clarity?

Its not needed now I think, it was because the Module was returning its
real dict and so there was a Module level instance with shared state
being used. This could leave subscriptions bound to all instances.
Module was changed to do the shallow copy of events in its init method
and this was removed.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode120
app/assets/javascripts/d3-components.js:120: var self = this,
On 2012/11/08 13:33:35, thiago wrote:
> It seems self is never used.

Done.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode124
app/assets/javascripts/d3-components.js:124: phase = 'on',
On 2012/11/08 18:49:46, matthew.scott wrote:
> phase is set and carefully handled, but not used.  Is this a future
> implementation thing?

It was intended to support before/after events. Of the three types  of
events this isn't handled quite properly anywhere yet.

The scene events are based on delegate which doesn't seem to support
phase (though I could filter out those things with non-on/after (which
maps to 'on' for DOM Nodes) and call Y.on. We'll see if we need it.

The d3 Events support before if we pass the capture flag to the handler
which maybe I should allow.

The custom events I will sort to properly support phase as this is the
primary target for this level of control.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode146
app/assets/javascripts/d3-components.js:146: if (typeof handler ===
'object') {
On 2012/11/08 13:33:35, thiago wrote:
> Null is also an object...

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/typeof

> Maybe you could use "Lang" for all the cases here.

Changed it up, thanks.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode155
app/assets/javascripts/d3-components.js:155: return;
On 2012/11/08 13:33:35, thiago wrote:
> If that is an error, you could throw the exception.

Its the sort of error I wanted a developer to notice but not one that I
wanted to stop processing of the entire component. I'm open to debate on
these two errors though.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode169
app/assets/javascripts/d3-components.js:169: for (var selector in
modEvents.scene) {
On 2012/11/08 13:33:35, thiago wrote:
> YUI utility?

Reworked, its passing the tests.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode191
app/assets/javascripts/d3-components.js:191: var resolvedHandler = {};
On 2012/11/08 13:33:35, thiago wrote:
> var declarations should be the first statement in the function body. I
dont know
> why lint didn't catch this.
> http://javascript.crockford.com/code.html

Moved

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode215
app/assets/javascripts/d3-components.js:215: var filtered = {};
On 2012/11/08 13:33:35, thiago wrote:
> The blocks of scope in js are crazy. The filtered variable is
available for use
> outside this if block. Maybe you could move it to the top of the
method to make
> it clear.

The structure here was the result of fighting with the linter from the
original form. I'll clean this up.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode220
app/assets/javascripts/d3-components.js:220:
Y.each(Y.Object.keys(eventSet), function _bind(name) {
On 2012/11/08 13:33:35, thiago wrote:
> Do you need to name this function ("_bind")?
Anonymous now.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode294
app/assets/javascripts/d3-components.js:294:
d3.selectAll(selector).on(name, null);
On 2012/11/08 13:33:35, thiago wrote:
> This method is really similar to the method above. Maybe you could
create a
> common method with a different handler for this inner "if"....

> function handleD3Events(modName, handler) {
> .
> .
> .
> }

They are similar but are called at different times with different APIs
(d3 events are handled post render). These methods are a bit more
compact after switching to Y.each style iteration as well. Hopefully
that makes people more comfortable.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode317
app/assets/javascripts/d3-components.js:317: var filtered = {};
On 2012/11/08 13:33:35, thiago wrote:
> One var declaration per closure.

Done.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode335
app/assets/javascripts/d3-components.js:335: var self = this;
On 2012/11/08 13:33:35, thiago wrote:
> You dont need 'self'. You pass the 'this' object as parameter to the
'Render
> Modules' line below.

The linter actually complained that 'this.method' to the containing
scope 'might' be a violation of 'strict', thus the change.

https://codereview.appspot.com/6828048/

-- 
https://code.launchpad.net/~bcsaller/juju-gui/component-framework/+merge/133391
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bcsaller/juju-gui/component-framework into lp:juju-gui.