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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
