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-components.js File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode44 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-components.js#newcode52 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-components.js#newcode70 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-components.js#newcode97 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-components.js#newcode124 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-components.js#newcode189 app/assets/javascripts/d3-components.js:189: // where object includes phase (before, on, after) From above, assuming this is the future use of phase? 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
