yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

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-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, [], {
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-components.js#newcode62
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-components.js#newcode85
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-components.js#newcode93
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-components.js#newcode95
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-components.js#newcode120
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-components.js#newcode146
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-components.js#newcode155
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-components.js#newcode160
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-components.js#newcode169
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-components.js#newcode191
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-components.js#newcode215
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-components.js#newcode220
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-components.js#newcode242
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-components.js#newcode247
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-components.js#newcode253
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-components.js#newcode294
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-components.js#newcode317
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-components.js#newcode335
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.

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.