This review is made by me and Nicola. Looking at the code we think we understood the logic of it all. This split of the env view into components and modules seems good, and allows composing the resulting scene in a more modular way.
However, a change of such scope and size really requires to be documented both in the code and in the project documentation. Your comment in this MP is a good start, and needs to be pushed into the docs and expanded/better explained. Furthermore, we agree with Benji on the need for quite a few more tests. Given the size of this proposal, it would be helpful to split this branch up into a series of smaller changes. Finally, we only see two tests running (without failures). Inline comments follow. https://codereview.appspot.com/6842084/diff/1/Makefile File Makefile (right): https://codereview.appspot.com/6842084/diff/1/Makefile#newcode100 Makefile:100: lint: gjslint jshint On 2012/11/21 20:43:27, benji wrote: > Was this intentional? Yeah, why did it have to go? https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-components.js File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-components.js#newcode97 app/assets/javascripts/d3-components.js:97: if (ModClassOrInstance === undefined) { Is this check here for some specific need, or is it some kind of debug leftover? https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-components.js#newcode364 app/assets/javascripts/d3-components.js:364: * and update(). If update requires some render state to operate on Please append a comma to this line. https://codereview.appspot.com/6842084/diff/1/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/environment.js#newcode31 app/views/environment.js:31: topo; topo in Italian means mouse ;-) https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js File app/views/topology/topology.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js#newcode9 app/views/topology/topology.js:9: * Topology models and renders the SVG of the envionment topology typo: environment. https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js#newcode23 app/views/topology/topology.js:23: options = options || {}; This line is a bit confusing. Isn't ``options`` a local var? Are these ``options`` used anywhere? https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js#newcode27 app/views/topology/topology.js:27: render: function() { Is this method override required? It seems to just invoke the superclass one without adding anything. https://codereview.appspot.com/6842084/ -- https://code.launchpad.net/~hazmat/juju-gui/component-modules/+merge/135501 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~hazmat/juju-gui/component-modules 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
