Topology Viewport module

Topology Viewport module

Enable viewport resizing via module.

https://code.launchpad.net/~bcsaller/juju-gui/viewport/+merge/142138

(do not edit description out of merge proposal)

[bcsaller, 2013-01-07 15:42:06]

Please take a look.

[gary.poster, 2013-01-07 17:52:57]

Land with changes.

Some very small suggestions below.

This test is not passing for me when I run the whole test suite.  In isolation
it is fine: http://pastebin.ubuntu.com/1507226/

I saw problems with zooming but you said it was pre-existing and something that
you and Brad are working on.  I'm OK with landing this if it's not making things
worse.

Thanks!

Gary

https://codereview.appspot.com/7071045/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7071045/diff/1/app/app.js#newcode754
app/app.js:754: // This alias doesn't seem to work, including refs by hand.
Bug-worthy?

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

https://codereview.appspot.com/7071045/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:330: * can trigger this after its sure
relevant elements
typo: it's

https://codereview.appspot.com/7071045/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/7071045/diff/1/app/views/environment.js#newcode73
app/views/environment.js:73: // Bind d3 events (manually).
Why do we have to do this here?  Worth a comment?

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

https://codereview.appspot.com/7071045/diff/1/app/views/topology/viewport.js#...
app/views/topology/viewport.js:42: // "afterPageSizeRecalculation" event at the
end of this function.
I suggest moving this comment to immediately before the pertinent code
("topo.fire('beforePageSizeRecalculation');")

https://codereview.appspot.com/7071045/diff/1/package.json
File package.json (right):

https://codereview.appspot.com/7071045/diff/1/package.json#newcode20
package.json:20: "yeti": ">=0.2.0",
we are not actually using this, are we?  If so, I suggest we delete it.

https://codereview.appspot.com/7071045/diff/1/test/index.html
File test/index.html (right):

https://codereview.appspot.com/7071045/diff/1/test/index.html#newcode31
test/index.html:31: <!-- Tests (Alphabetical)-->
Cool, thanks :-)

[bcsaller, 2013-01-08 15:26:45]

Please take a look.

[matthew.scott, 2013-01-08 18:38:47]

We discussed the lack of scroll-wheel capability on the call (something I
apparently relied on quite a bit, judging by how I did my functional testing -
perhaps a relatively high priority on that).  

However, another regression that I noticed in testing was the fact that menus
are not positioned properly, nor do they follow the service they're attached to
when zooming or panning; they remain at (0,0).  uistage shows the service menu
remaining open and following the service during both pan and zoom events, no
matter the source.

I'm not comfortable having two user-visible regressions in one branch - I'm fine
with the scroll-wheel one because that one isn't necessarily discoverable,
though it's certainly high on my own personal list, but I think this second one
is a little too visibly not right.  Just one minor in code, so far, though I'll
give it a more thorough looking-over later today (it's hard to tell all that
changed with these refactors).

Thanks for the branch!

https://codereview.appspot.com/7071045/diff/9001/app/assets/javascripts/d3-co...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/7071045/diff/9001/app/assets/javascripts/d3-co...
app/assets/javascripts/d3-components.js:203: //console.debug('Handler for',
name, selector, d3.event);
Could be removed or just uncommented, unless it causes too much clutter in the
console.

https://codereview.appspot.com/7071045/diff/9001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/7071045/diff/9001/app/views/environment.js#new...
app/views/environment.js:71: rendered: function() {
Good rename, I think.

https://codereview.appspot.com/7071045/diff/9001/app/views/topology/viewport.js
File app/views/topology/viewport.js (right):

https://codereview.appspot.com/7071045/diff/9001/app/views/topology/viewport....
app/views/topology/viewport.js:43: resized: function() {
Overall a great simplification.  I like it!

[bcsaller, 2013-01-09 03:32:31]

On 2013/01/08 18:38:47, matthew.scott wrote:
> We discussed the lack of scroll-wheel capability on the call (something I
> apparently relied on quite a bit, judging by how I did my functional testing -
> perhaps a relatively high priority on that).  
> 
> However, another regression that I noticed in testing was the fact that menus
> are not positioned properly, nor do they follow the service they're attached
to
> when zooming or panning; they remain at (0,0).  uistage shows the service menu
> remaining open and following the service during both pan and zoom events, no
> matter the source.
> 
> I'm not comfortable having two user-visible regressions in one branch - I'm
fine
> with the scroll-wheel one because that one isn't necessarily discoverable,
> though it's certainly high on my own personal list, but I think this second
one
> is a little too visibly not right.  Just one minor in code, so far, though
I'll
> give it a more thorough looking-over later today (it's hard to tell all that
> changed with these refactors).
> 
> Thanks for the branch!
> 
>
https://codereview.appspot.com/7071045/diff/9001/app/assets/javascripts/d3-co...
> File app/assets/javascripts/d3-components.js (right):
> 
>
https://codereview.appspot.com/7071045/diff/9001/app/assets/javascripts/d3-co...
> app/assets/javascripts/d3-components.js:203: //console.debug('Handler for',
> name, selector, d3.event);
> Could be removed or just uncommented, unless it causes too much clutter in the
> console.
> 
> https://codereview.appspot.com/7071045/diff/9001/app/views/environment.js
> File app/views/environment.js (right):
> 
>
https://codereview.appspot.com/7071045/diff/9001/app/views/environment.js#new...
> app/views/environment.js:71: rendered: function() {
> Good rename, I think.
> 
>
https://codereview.appspot.com/7071045/diff/9001/app/views/topology/viewport.js
> File app/views/topology/viewport.js (right):
> 
>
https://codereview.appspot.com/7071045/diff/9001/app/views/topology/viewport....
> app/views/topology/viewport.js:43: resized: function() {
> Overall a great simplification.  I like it!

Nice catch on the menu. I tried to better encapsulate pan/zoom but didn't track
down all the usage. Fix applied.

The debug console log was pretty chatty but I can put it back on if people want.

I'd be more than happy to spend a little time with out trying to get the mouse
translate stuff working again but I'd rather do that in another branch (I used
it exclusively in the past).

Thanks for the review.

[bcsaller, 2013-01-09 17:02:34]

*** Submitted:

Topology Viewport module

Enable viewport resizing via module.

R=gary.poster, matthew.scott
CC=
https://codereview.appspot.com/7071045