Fix "original" scale limits with nonlinear pan#774
Merged
Conversation
These currently raise spurious errors, due to limitations in Chart.js types.
Nonlinear scales failed to resolve "original" within panNumericalScale. To fix this: * Resolving "original" within panNumericalScale seemed redundant, when updateRange was already doing it, so I instead moved the logic into updateRange and control it with a new special value for the `zoom` parameter. (Hopefully the use of a special string like this is acceptable as long as JSDoc calls it out - if I should use another approach, please let me know.) * This change put updateRange over the configured ESLint complexity limit, so I extracted a new getScaleLimits function to reduce complexity. Add unit tests, both for this bug and for the preexisting but untested "original" feature.
joshkel
added a commit
to joshkel/Chart.js
that referenced
this pull request
Jul 25, 2023
While adding some type definitions to chartjs-plugin-zoom (see chartjs/chartjs-plugin-zoom#774), I noticed a few limitations in Chart.js's scale types: * The zoom plugin calls `Scale.parse` with no index parameter. Scale's JSDoc allows this, but its TypeScript definitions did not. * The zoom plugin alters scale options' min and max. The specific types of these depend on which scale is in use, but every scale has them, so `unknown` seems appropriate
etimberg
pushed a commit
to chartjs/Chart.js
that referenced
this pull request
Jul 25, 2023
While adding some type definitions to chartjs-plugin-zoom (see chartjs/chartjs-plugin-zoom#774), I noticed a few limitations in Chart.js's scale types: * The zoom plugin calls `Scale.parse` with no index parameter. Scale's JSDoc allows this, but its TypeScript definitions did not. * The zoom plugin alters scale options' min and max. The specific types of these depend on which scale is in use, but every scale has them, so `unknown` seems appropriate
kurkle
approved these changes
Jul 27, 2023
Contributor
|
This is merged into my fork at https://github.com/trullock/chartjs-plugin-zoom which is published on npm as @trullock/chartjs-plugin-zoom, as this fork seems abandoned |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nonlinear scales failed to resolve "original" within panNumericalScale.
To fix this:
zoomparameter. (Hopefully the use of a special string like this is acceptable as long as JSDoc calls it out - if I should use another approach, please let me know.)Add unit tests, both for this bug and for the preexisting but untested "original" feature.
I wanted JSDoc + TypeScript checks to help validate and document these changes, but that exposed apparent limitations of Chart.js core's type definitions. I'll open a separate PR for those.