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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
