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

Default scale should be adjusted if beyond given min/max scale (was: Zoom out not working) #138

Open
cookiejest opened this issue Apr 29, 2014 · 5 comments

Comments

@cookiejest
Copy link

The zoom out button seems to have stopped working, even in the demo site.

@usmonster usmonster added the bug label Apr 29, 2014
@usmonster
Copy link
Collaborator

Whoa, you're absolutely correct--good catch, thanks! Should be an easy fix, though might need to wait a week or so before I can take a look. Feel free to submit a patch before then if you're so inclined!

@cookiejest
Copy link
Author

Can do.. Can you point me towards the solution? I'm not that familiar with the code!

Adam

Sent from my iPhone

On 29 Apr 2014, at 13:36, usmonster [email protected] wrote:

Whoa, you're absolutely correct--good catch, thanks! Should be an easy fix, though might need to wait a week or so before I can take a look. Feel free to submit a patch before then if you're so inclined!


Reply to this email directly or view it on GitHub.

@usmonster
Copy link
Collaborator

@cookiejest I would point you to the zoomInOut function, but I would first suggest that you try to reproduce the issue using the most recent version of the plugin from the master branch. The version on the demo site is often behind.

@cookiejest
Copy link
Author

I have checked that function you suggest against an older version I have where zoom out is working and it looks identical.

Also checked the function against the latest version and doesn't look like it's changed so it must be something else?

Not sure I'm advanced enough to work this out. :S

@usmonster
Copy link
Collaborator

Hi @cookiejest! I just had a chance to look, and it seems that there are two issues at play here:

The first issue is that there's currently no sanity check to make sure the initial/default scale option doesn't violate the max/min scales, which is currently the case on the demo page (introduced in 5a52561). A minimal fix for this would be to adjust the default scale to be the closest of either maxScale or minScale (but only if the user doesn't already supply the scale option). Feel free to put this into a pull request, though it might be just as well to wait until #84 lands, as that will surely touch the same code.

The other issue is that the semantics of the maxScale/minScale options can seem kind of counterintuitive (I might actually say "wrong").. In this plugin, minScale denotes in fact the minimum time unit that you want the chart to display; i.e., how "close" you want to be able to "zoom in" to the timeline, or in other words, the scale with the maximum(!) zoom factor. maxScale does the opposite -- basically it's the limit for how far you can zoom out / the largest displayed time unit, i.e., the scale that has the minimum zoom factor. Yeah, that doesn't really make a lot of sense...

We could maybe deprecate those options and introduce new options with clearer names (minScale => zoomMax, maxScale => zoomMin?), but a more immediate minimal patch would be to just document this behavior. This, paired with the recommended fix to the default scale issue, would be a nice way to get your hands dirty and start contributing if you like! Otherwise there should be patches in the coming weeks.

Thanks again for reporting this!

@usmonster usmonster changed the title Zoom out not working Default scale should be adjusted if beyond given min/max scale (was: Zoom out not working) Dec 1, 2014
@usmonster usmonster added this to the v1.2.0 milestone Dec 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants