Code review comment for lp://staging/~rharding/juju-gui/renderBundle2

Revision history for this message
Richard Harding (rharding) wrote :

Reviewers: mp+182735_code.launchpad.net,

Message:
In general this looks good. Couple of nit-picks below. I'm not
comfortable with so much code dupe between bundle.js and service.js
though so not ok'ing all the way.

https://codereview.appspot.com/13361043/diff/1/app/assets/javascripts/d3-components.js
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/13361043/diff/1/app/assets/javascripts/d3-components.js#newcode290
app/assets/javascripts/d3-components.js:290: if (this.get('interactive')
=== false) { return; }
what is the interactive attr? I don't see it defined and not following
what this is doing.

https://codereview.appspot.com/13361043/diff/1/app/views/topology/bundle.js
File app/views/topology/bundle.js (right):

https://codereview.appspot.com/13361043/diff/1/app/views/topology/bundle.js#newcode80
app/views/topology/bundle.js:80: //Process any changed data.
space after the //

https://codereview.appspot.com/13361043/diff/1/app/views/topology/bundle.js#newcode87
app/views/topology/bundle.js:87: // enter
enter? more or remove?

https://codereview.appspot.com/13361043/diff/1/app/views/topology/bundle.js#newcode92
app/views/topology/bundle.js:92: return (d.subordinate ? 'subordinate '
: '') +
so is this repeated? Can it be shared? A typology.utils helper or
something?

https://codereview.appspot.com/13361043/diff/1/test/test_topology.js
File test/test_topology.js (right):

https://codereview.appspot.com/13361043/diff/1/test/test_topology.js#newcode135
test/test_topology.js:135: // The size of the element shoul reflect teh
passed in params
the

Description:
WIP

https://code.launchpad.net/~rharding/juju-gui/renderBundle2/+merge/182735

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13361043/

Affected files:
   A [revision details]
   M app/app.js
   M app/assets/javascripts/d3-components.js
   M app/modules-debug.js
   A app/views/topology/bundle.js
   M app/views/topology/relation.js
   M app/views/topology/topology.js
   A test/data/wp-deployer.yaml
   M test/test_topology.js

« Back to merge proposal