Service health graphs sized properly on update.

Service health graphs sized properly on update.

Service health graphs were not being resized properly due to d3's enter() not noticing that the values had changed.  This is fixed, and may also fix https://bugs.launchpad.net/juju-gui/+bug/1098589 in the process, depending on whether or not we can duplicate that issue.

https://code.launchpad.net/~makyo/juju-gui/service-health-graph-update/+merge/143399

(do not edit description out of merge proposal)

[matthew.scott, 2013-01-15 20:37:30]

Please take a look.

[gary.poster, 2013-01-15 21:19:14]

Land with (super trivial) changes.

Thank you!  Looks good to me, tests pass, and it is very nice to have the fix
and new tests.

Gary

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

https://codereview.appspot.com/7095061/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:403: updateServiceNodes: function(node) {
In some future iteration, it sure would be nice to divide this function up for
easier reading and maintainability.  No worries for this branch at all, but it's
hard to figure out what is going on in code like this IME.  (I should know, I've
written some code like this that would make you cry :-) )

https://codereview.appspot.com/7095061/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:548: .attr('data-outerradius', outerRadius);
As we discussed on IRC, I normally would like to separate out code that
calculates values from code that mutates in a context like this one, but you
said this attribute was only for testability.  In that case, please just add a
comment explaining the purpose if data-outerradius.

https://codereview.appspot.com/7095061/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7095061/diff/1/test/test_environment_view.js#n...
test/test_environment_view.js:255: var node = d3.select(serviceNode);
Sigh.  The indentation rules still surprise me. :-/  I would have expected the
beautifier/linter to ask you to have a two space indent.  Do what you will. :-)

[bcsaller, 2013-01-15 22:37:35]

LGTM, Thanks!

[matthew.scott, 2013-01-15 22:53:39]

Thanks for the reviews!

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

https://codereview.appspot.com/7095061/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:548: .attr('data-outerradius', outerRadius);
On 2013/01/15 21:19:14, gary.poster wrote:
> As we discussed on IRC, I normally would like to separate out code that
> calculates values from code that mutates in a context like this one, but you
> said this attribute was only for testability.  In that case, please just add a
> comment explaining the purpose if data-outerradius.

Done.

https://codereview.appspot.com/7095061/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7095061/diff/1/test/test_environment_view.js#n...
test/test_environment_view.js:255: var node = d3.select(serviceNode);
On 2013/01/15 21:19:14, gary.poster wrote:
> Sigh.  The indentation rules still surprise me. :-/  I would have expected the
> beautifier/linter to ask you to have a two space indent.  Do what you will.
:-)

make prep did this the first time, but I tried forcing it back, and then it was
suddenly okay with it.  Non-deterministic beautifier?

[matthew.scott, 2013-01-15 22:57:03]

*** Submitted:

Service health graphs sized properly on update.

Service health graphs were not being resized properly due to d3's enter() not
noticing that the values had changed.  This is fixed, and may also fix
https://bugs.launchpad.net/juju-gui/+bug/1098589 in the process, depending on
whether or not we can duplicate that issue.

R=gary.poster, bcsaller
CC=
https://codereview.appspot.com/7095061