← Back to team overview

yellow team mailing list archive

Re: Topology Viewport module (issue 7071045)

 

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-components.js
> File app/assets/javascripts/d3-components.js (right):


https://codereview.appspot.com/7071045/diff/9001/app/assets/javascripts/d3-components.js#newcode203
> 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#newcode71
> 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.js#newcode43
> 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.


https://codereview.appspot.com/7071045/

-- 
https://code.launchpad.net/~bcsaller/juju-gui/viewport/+merge/142138
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bcsaller/juju-gui/viewport into lp:juju-gui.


References