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

Fix indexScale pan/zoom proportion #151 #152

Closed
wants to merge 1 commit into from

Conversation

subogero
Copy link

Plus stop zooming on indexScale when panning.

Plus stop zooming on indexScale when panning.
@subogero
Copy link
Author

subogero commented Aug 1, 2018

Is this plugin officially abandonware?

ColemanCollins added a commit to ColemanCollins/chartjs-plugin-zoom that referenced this pull request Aug 14, 2018
@benmccann
Copy link
Collaborator

@subogero sorry for the long delay in reviewing. This PR will need to be rebased against master

newMin = scale.getValueForPixel(scale.getPixelForValue(start) - delta);
newMax = scale.getValueForPixel(scale.getPixelForValue(end) - delta);

// Dont scale if either limit is reached
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont -> Don't


zoomNS.panCumulativeDelta += delta;
var dIndex = scale.getValueForPixel(Math.abs(delta)) - scale.getValueForPixel(0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is dIndex short for deltaIndex? If so, I'd prefer that as a variable name to be a bit clearer

if (tickOpts.reverse) {
tickOpts.max = scale.getValueForPixel(scale.getPixelForValue(start) - delta);
tickOpts.min = scale.getValueForPixel(scale.getPixelForValue(end) - delta);
var rangeMin, rangeMax
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like maybe we can replace the logic from here down with a call to rangeMinLimiter and rangeMaxLimiter?

@benmccann
Copy link
Collaborator

@subogero thanks for this PR. And sorry again for the delay in reviewing. I'm trying to get all the pending PRs merged. I know it's been awhile. If you could let me know if you're still interested in this PR or not that would be helpful, so that I know whether to wait to cut a release to include this one. Thanks again

@benmccann
Copy link
Collaborator

@subogero just checking in on this to see if you're still interested in pursuing. Sorry again for the long wait

@benmccann
Copy link
Collaborator

@subogero just a reminder that this PR will need to be rebased. Are you still interested in getting it in?

@kurkle kurkle added the bug label Mar 12, 2021
@kurkle
Copy link
Member

kurkle commented Mar 12, 2021

Needs a rebase. The proportion part was already merged, but the stop zooming part would still be valid fix.

@kurkle
Copy link
Member

kurkle commented May 1, 2021

Closing as everything in this has already been implemented

@kurkle kurkle closed this May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants