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

Issue 6856067: Fix initial creation of subordinates.

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by matthew.scott
Modified:
11 years, 5 months ago
Reviewers:
mp+135022
Visibility:
Public.

Description

Fix initial creation of subordinates. When creating a subordinate service initially, it was not being laid out properly by the pack layout, nor was it seeming to have drag events attached. Pack layout was expecting a value accessor function to return some number greater than zero when constructing its radius, which informs the x/y coordinates. Returning zero results in NaN for all three values, thus positioning the service at (0,0) and causing drag events to fail when updating the service's coordinates with dx/dy + NaN. This branch fixes this by ensuring that zero is never returned as the node's value. This fix is quick and intended to make the UI function with the current layout algorithm. Future layout algorithms, if they're designed by hand, will need to keep this in mind as well. https://code.launchpad.net/~makyo/juju-gui/adding-subordinates/+merge/135022 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix initial creation of subordinates. #

Patch Set 3 : Fix initial creation of subordinates. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -1 line) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M app/views/environment.js View 1 1 chunk +3 lines, -1 line 0 comments Download
M test/test_environment_view.js View 1 1 chunk +47 lines, -0 lines 0 comments Download

Messages

Total messages: 7
matthew.scott
Please take a look.
11 years, 5 months ago (2012-11-20 00:13:38 UTC) #1
benjamin.saller
The diff looks fine, but there is behavior we expect on subordinates that isn't be ...
11 years, 5 months ago (2012-11-20 13:31:33 UTC) #2
bac
Looks good Matt. Thanks. https://codereview.appspot.com/6856067/diff/1/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6856067/diff/1/app/views/environment.js#newcode323 app/views/environment.js:323: }) maybe this would be ...
11 years, 5 months ago (2012-11-20 14:50:48 UTC) #3
matthew.scott
A test was added as well, but rather than just testing whether or not subordinates ...
11 years, 5 months ago (2012-11-20 15:59:08 UTC) #4
matthew.scott
Please take a look.
11 years, 5 months ago (2012-11-20 16:26:44 UTC) #5
bac
The new test makes sense to me Matt.
11 years, 5 months ago (2012-11-21 17:27:11 UTC) #6
matthew.scott
11 years, 5 months ago (2012-11-21 17:35:56 UTC) #7
*** Submitted:

Fix initial creation of subordinates.

When creating a subordinate service initially, it was not being laid out
properly by the pack layout, nor was it seeming to have drag events attached. 
Pack layout was expecting a value accessor function to return some number
greater than zero when constructing its radius, which informs the x/y
coordinates.  Returning zero results in NaN for all three values, thus
positioning the service at (0,0) and causing drag events to fail when updating
the service's coordinates with dx/dy + NaN.  This branch fixes this by ensuring
that zero is never returned as the node's value.  This fix is quick and intended
to make the UI function with the current layout algorithm.  Future layout
algorithms, if they're designed by hand, will need to keep this in mind as well.

R=benjamin.saller, bac
CC=
https://codereview.appspot.com/6856067
Sign in to reply to this message.

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