Micro Framework for Environment View

Micro Framework for Environment View

A subclass of Component will container 1 or more modules 
which implement their own areas of application concern.

The pattern is that we can declaratively define event listeners
and respond to those across modules. For example A canvas click
might fire an application event 'clickedCanvas' which each module
can have subscribers for that do things like close menus and remove
drag lines, etc.

https://code.launchpad.net/~bcsaller/juju-gui/component-framework/+merge/133391

(do not edit description out of merge proposal)

[bcsaller, 2012-11-08 01:53:16]

Please take a look.

[thiago, 2012-11-08 13:33:35]

Hi Ben!

Thanks for this branch. It seems you had a lot of fun doing this. :O)

This is a review for your main file. I still need to look at the tests.

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

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:40: var Component =
Y.Base.create('Component', Y.Base, [], {
I like this code style. You could the same with the class above. I mean...

var Module = ...

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:62: // this.after('containerChange',
this.bind());
You could remove the commented code.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:85: options = options || {};
I don't like setting a value to one function parameter.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:93: this.modules[module.name] = module;
Funny indentation

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:95: var modEvents = module.events;
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.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:120: var self = this,
It seems self is never used.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:146: if (typeof handler === 'object') {
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.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:155: return;
If that is an error, you could throw the exception.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:160: return;
If that is an error, you could throw the exception.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:169: for (var selector in
modEvents.scene) {
YUI utility?

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:191: var resolvedHandler = {};
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

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:215: var filtered = {};
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.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:220: Y.each(Y.Object.keys(eventSet),
function _bind(name) {
Do you need to name this function ("_bind")?

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:242: if (!modEvents || modEvents.d3 ===
undefined) {
If modEvents.d3 is null you will have a false negative. Is that what you want?
If not, you could change it to...

if (!modEvents || !modEvents.d3)

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:247: var module = this.modules[modName],
Usually we have one "var" per closure.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:253: for (selector in modEvents) {
What about the yui utility classes? You could use Y.Object.keys, no?

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:294: d3.selectAll(selector).on(name,
null);
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) {
.
.
.
}

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:317: var filtered = {};
One var declaration per closure.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:335: var self = this;
You dont need 'self'. You pass the 'this' object as parameter to the 'Render
Modules' line below.

[thiago, 2012-11-08 14:13:01]

https://codereview.appspot.com/6828048/diff/1/test/test_d3_components.js
File test/test_d3_components.js (right):

https://codereview.appspot.com/6828048/diff/1/test/test_d3_components.js#newc...
test/test_d3_components.js:65: comp.addModule(TestModule);
Nothing to assert?

[matthew.scott, 2012-11-08 18:49:45]

This all looks pretty good to me, with a few little minors/clarifications and
Thiago's comments.  Eager to see it plugged in, for sure.

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

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:44: * Component collections modules
implementing various portions
Minor - should be 'collects', I think?  Was just confusing.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:52: *    - Providing suggestions around
updating the interactive portions
Minor - 'updating'

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:70: *
Minor - Docs don't match method signature.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:97: this.bind(module.name);
Curious about the necessity for not doing Y.clone(module.events); here. 
Clarity?

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:124: phase = 'on',
phase is set and carefully handled, but not used.  Is this a future
implementation thing?

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:189: //       where object includes
phase (before, on, after)
From above, assuming this is the future use of phase?

[benjamin.saller, 2012-11-09 13:39:21]

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-compo...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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-compo...
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.

[bcsaller, 2012-11-09 13:41:47]

Please take a look.

[matthew.scott, 2012-11-09 18:07:59]

Thanks for the changes!  Looks good to me.

[thiago, 2012-11-09 19:30:12]

On 2012/11/09 18:07:59, matthew.scott wrote:
> Thanks for the changes!  Looks good to me.

Thanks, Ben. It looks good to me too.

[bcsaller, 2012-11-13 14:10:27]

*** Submitted:

Micro Framework for Environment View

A subclass of Component will container 1 or more modules 
which implement their own areas of application concern.

The pattern is that we can declaratively define event listeners
and respond to those across modules. For example A canvas click
might fire an application event 'clickedCanvas' which each module
can have subscribers for that do things like close menus and remove
drag lines, etc.

R=thiago, matthew.scott, benjamin.saller
CC=
https://codereview.appspot.com/6828048