Skip to content

Conversation

@matthull
Copy link
Contributor

This eliminates the behavior where chart pans left and right when brush is manipulated while mouse zoom is enabled.

Also made mouseZoomable() be evaluated on each re-render to avoid the situation where changes to mouseZoomable() take effect on brushing but not on re-rendering.

Downside of this change is that you lose ability to pan by dragging when brush is enabled.

@jrideout
Copy link
Contributor

Per the discussion in #253. This is on hold for merging until we figure out the overall approach as described in https://github.com/NickQiZhu/dc.js/wiki/Zoom-Behaviors-Combined-with-Brush-and-Range-Chart

@jrideout
Copy link
Contributor

@matthull I've started working on cleaning up some of the other todo items against your branch. I'm not sure it is the right direct yet - but here is my work in progress:

https://github.com/NickQiZhu/dc.js/compare/fix-zoom

@matthull
Copy link
Contributor Author

@jrideout Good stuff!

While looking at your changes, I just noticed what may be another existing issue: unless I'm overlooking something, focusChart's invokeZoomedListener never gets invoked when rangeChart gets manipulated. Same is true when one chart focuses another as in focus() documentation example. Just did a quick test and this looks to be the case.

It seems like cheating that you can invoke focus(), which in this context is a synonym for 'zoom', without the zoomHandler being involved in the fun. zoomHandler might get a bit jealous.

I don't have a clear vision at this moment for exactly how it could work, but seems like focus() should somehow invoke zoomHandler() (either directly or via zoom.event()), not the other way around as currently implemented. This would ensure that all zoom concerns such as invokeZoomedListener are handled consistently regardless of whether zoomHandler is invoked due to event like mousewheel, or by someone programatically calling focus().

@jrideout
Copy link
Contributor

@matthull good catch on invokeZoomedListener. I probably won't have time to tinker on the the zoom issues for another week or so. So, if you want to retake the lead and rework what I started along with your changes please go ahead.

@matthull
Copy link
Contributor Author

@jrideout Groovy, I'll take a stab at it over the next couple days.

@matthull
Copy link
Contributor Author

matthull commented Nov 2, 2013

@jrideout I made some changes on top of your zoom-fix branch over here. Still very much work in progress - but check it out if you have a sec and let me know if it looks like an OK direction. Closing this PR since it is superseded by the more comprehensive overhaul.

@matthull matthull closed this Nov 2, 2013
@jrideout
Copy link
Contributor

jrideout commented Nov 4, 2013

Comparison link for reference:

matthull/dc.js@NickQiZhu:master...fix-zoom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants