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

Multi-axis zoom and original zoom limits #524

Merged
merged 7 commits into from
Jun 2, 2021
Merged

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented May 28, 2021

This fixes #522 and #523. I put it together for my own use, so everything's in one branch, but I'm happy to submit as smaller PRs for discussion and review whenever someone's ready to take a look at it.

Thank you.

@kurkle
Copy link
Member

kurkle commented May 28, 2021

Had a quick look, looks good. The .gitignore changes should be split off, if those are needed at all. Not a fan of passing the state around that much (actually just the number of args), but I think its better than getting the state everywhere.

@kurkle
Copy link
Member

kurkle commented Jun 1, 2021

Something is broken:

[test-karma] ✗ /base/test/fixtures/zoom/zoom-reset.js
[test-karma] RangeError: invalid digits value: NaN thrown

This should fix test failures.
@joshkel
Copy link
Contributor Author

joshkel commented Jun 1, 2021

Thanks. I fixed the test failure.

I wasn't a big fan of the number of parameters, either, but I didn't see a clean way of reducing it.

I noticed that the module exports zoomFunctions and panFunctions, so that third-party custom scale classes can register specialized handling for zoom and pan (#169). This PR would change the function signature of those functions, which would be a SemVer breaking change. Should I move the chartState parameter to the end of each of the zoomFunctions and panFunctions to avoid this? Or, since the issue is still open and the feature is not yet documented, is it okay to make changes?

@kurkle
Copy link
Member

kurkle commented Jun 1, 2021

Good catch the breaking change on zoomFunctions and panFunctions.

Maybe the extra parameters should be avoided at least on those functions, as the getState should be fast enough to just call again in there.

src/core.js Outdated Show resolved Hide resolved
src/core.js Outdated Show resolved Hide resolved
This lets us keep the `zoomFunctions` and `panFunctions` signatures unchanged, since they're part of the public API.
@joshkel
Copy link
Contributor Author

joshkel commented Jun 1, 2021

Thanks for the feedback.

If I avoid the extra parameter for zoomFunctions and panFunctions and instead retrieve the state within the updateRange function that they call, then most of the changes that I'd originally made end up being unnecessary.

So it's probably still a bit cleaner to call getState once within the top-level public API methods, and use it in places like this, and pass it to storeOriginalScaleLimits, but it's no longer needed for this PR. Would you like me to go ahead and do it?

@kurkle
Copy link
Member

kurkle commented Jun 1, 2021

I'd be ok having it done in this pr, if you are willing to do it ;)

As discussed in code review, it seems cleaner to consistently get state at the start of a function then pass it to `storeOriginalScaleLimits`.
@joshkel
Copy link
Contributor Author

joshkel commented Jun 2, 2021

@kurkle Done. Thanks.

@kurkle kurkle added this to the 1.1.0 milestone Jun 2, 2021
@kurkle kurkle merged commit bb7b8fc into chartjs:master Jun 2, 2021
@joshkel joshkel deleted the multi-axis branch June 2, 2021 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zoom limits with multiple axes
2 participants