Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added caution message for mouse-zoomable conflict with elasticX #1623

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

added caution message for mouse-zoomable conflict with elasticX #1623

wants to merge 1 commit into from

Conversation

privateOmega
Copy link

@privateOmega privateOmega commented Nov 25, 2019

@gordonwoodhull I found that I was unable to zoom-in to my graph even with mouseZoomable(true) and documentation didn't explicity specify this case.

After some googling I found your answer in SO, and I thought users in future shouldn't be wasting time on this. The commit contains update in documentation and rebuild docs.

This can be removed once the below is the case.

dc.js should only "elast" the X domain once (on render) if both options are set

@gordonwoodhull
Copy link
Contributor

Thanks @privateOmega! Related: #987, elasticX is not compatible with focus chart.

I am tempted to do what is probably expected and turn off elasticX after the first render, in either case.

(Or perhaps, since it would also be surprising for a parameter to change on its own, define elasticX as only active on render not redraw, if either of these features are enabled.)

@privateOmega
Copy link
Author

@gordonwoodhull Since you have plans to fix it. I am abandoning the PR.

@gordonwoodhull
Copy link
Contributor

I’m going to reopen this as a reminder to fix this. I’m currently on vacation but I think it will be a quick fix when I get back.

Otherwise I think we would have to document it in at least three places!

@privateOmega
Copy link
Author

@gordonwoodhull Sure. Sounds perfect.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Dec 13, 2019

I started this on the elasticX-only-render branch, if anyone wants to try it out.

So far I am seeing focus charts getting set to [x1, 1] instead of [x1, x2], and getting reset to either [0, 1] or [1, 1] when the selection is cleared, so I clearly missed something.

Besides calculating the domain only on render, it also sets _originalDomain when rendered. (mouseZoomable charts will not allow the domain to exceed the original domain, and also return to the original domain when the focus is cleared.)

Everything works and I have converted many of the examples to use elasticX instead of setting the domain manually.

I did see a performance regression on the "focus dynamic interval" example, which I'll need to look into before releasing this (as 3.2, to be merged into 4.0, also to be released soon).

gordonwoodhull added a commit that referenced this pull request Dec 16, 2019
elasticX can reasonably be interpreteted as "just calculate the domain for me, unless it's forced by something else"
so ignore it during redraw if this is a focus chart or mouseZoomable is active
fixes #1623, #987
gordonwoodhull added a commit that referenced this pull request Dec 16, 2019
elasticX can reasonably be interpreteted as "just calculate the domain for me, unless it's forced by something else"
so ignore it during redraw if this is a focus chart or mouseZoomable is active
fixes #1623, #987
gordonwoodhull added a commit that referenced this pull request Dec 22, 2019
elasticX can reasonably be interpreteted as "just calculate the domain for me, unless it's forced by something else"
so ignore it during redraw if this is a focus chart or mouseZoomable is active
fixes #1623, #987
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