Merge lp://staging/~rharding/juju-gui/renderBundle2 into lp://staging/juju-gui/experimental

Proposed by Richard Harding
Status: Work in progress
Proposed branch: lp://staging/~rharding/juju-gui/renderBundle2
Merge into: lp://staging/juju-gui/experimental
Diff against target: 623 lines (+496/-12)
8 files modified
app/app.js (+1/-0)
app/assets/javascripts/d3-components.js (+3/-0)
app/modules-debug.js (+3/-0)
app/views/topology/bundle.js (+390/-0)
app/views/topology/relation.js (+3/-0)
app/views/topology/topology.js (+15/-12)
test/data/wp-deployer.yaml (+41/-0)
test/test_topology.js (+40/-0)
To merge this branch: bzr merge lp://staging/~rharding/juju-gui/renderBundle2
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+182735@code.staging.launchpad.net

Description of the change

To post a comment you must log in.
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

Revision history for this message
Madison Scott-Clary (makyo) wrote :

LGTM, QA okay

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#newcode38
app/views/topology/bundle.js:38: Manage service rendering and events.
s/service/bundle

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
On 2013/08/28 19:47:30, rharding wrote:
> the

should

https://codereview.appspot.com/13361043/

Unmerged revisions

988. By Richard Harding

Sync

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches