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