| Thread Previous • Date Previous • Date Next • Thread Next |
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.
| Thread Previous • Date Previous • Date Next • Thread Next |
