Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1923)

Issue 6842084: Ben's branch for env refactoring

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by hazmat
Modified:
11 years, 4 months ago
Reviewers:
frankban, benji, mp+135501
Visibility:
Public.

Description

Ben's branch for env refactoring Ben's branch for env refactoring... testing https://code.launchpad.net/~hazmat/juju-gui/component-modules/+merge/135501 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+850 lines, -1808 lines) Patch
M Makefile View 1 chunk +1 line, -1 line 2 comments Download
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M app/assets/javascripts/d3-components.js View 8 chunks +56 lines, -11 lines 4 comments Download
M app/modules-debug.js View 2 chunks +27 lines, -0 lines 0 comments Download
M app/templates/overview.handlebars View 1 chunk +2 lines, -2 lines 0 comments Download
M app/views/environment.js View 2 chunks +33 lines, -1776 lines 3 comments Download
A app/views/topology/panzoom.js View 1 chunk +42 lines, -0 lines 1 comment Download
A app/views/topology/relation.js View 1 chunk +42 lines, -0 lines 0 comments Download
A app/views/topology/service.js View 1 chunk +361 lines, -0 lines 2 comments Download
A app/views/topology/topology.js View 1 chunk +133 lines, -0 lines 4 comments Download
A app/views/topology/viewport.js View 1 chunk +41 lines, -0 lines 0 comments Download
M test/index.html View 1 chunk +1 line, -0 lines 0 comments Download
M test/test_d3_components.js View 3 chunks +8 lines, -4 lines 0 comments Download
M test/test_environment_view.js View 4 chunks +4 lines, -14 lines 0 comments Download
A test/test_topology.js View 1 chunk +97 lines, -0 lines 1 comment Download

Messages

Total messages: 3
hazmat
Please take a look.
11 years, 4 months ago (2012-11-21 19:25:59 UTC) #1
benji
I do not understand the intent of this branch, so my opinions can not realistically ...
11 years, 4 months ago (2012-11-21 20:43:27 UTC) #2
frankban
11 years, 4 months ago (2012-11-22 17:55:57 UTC) #3
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-compo...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-compo...
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-compo...
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#...
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#...
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#...
app/views/topology/topology.js:27: render: function() {
Is this method override required? It seems to just invoke the superclass one
without adding anything.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b