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

Fixes plugin on Chart.js 3.5.x / 3.6.x. #88

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TimUnderhay
Copy link

Fixes plugin with Chart.js 3.5.x / 3.6.x.
Allows X axis scale to use either 'x' or 'xAxes'.

Allows X axis scale to use either 'x' or 'xAxes'.
Update for 'xAxes' scale name.
Comment on lines 378 to +380
// set axis scale
chart.options.scales.x.min = start;
chart.options.scales.x.max = end;
chart.options.scales[xAxisName].min = start;
chart.options.scales[xAxisName].max = end;
Copy link

Choose a reason for hiding this comment

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

Consider that if "xAxisName" is "xAxes" then it's an array, not an single object

Choose a reason for hiding this comment

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

This is not true, all scales in chart.js V3 are single objects

Copy link

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

this is not a flexibale way of solving that you have given your x axis scale a different ID as the default, more flexibel way would be to allow a option field in the plugin options where this can be configured, that defaults to chart.js's default which is x


if (!chart.config.options.scales.x) {
return
const xScaleOptions = chart.config.options.scales.x || chart.config.options.scales.xAxes;

Choose a reason for hiding this comment

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

Would be better to make this a variable that the user can configure in the options for the plugin so that all names you give to your x axis are supported.

Then if no name has been given you can fall back to chart.js's default x axis scale id which is x

Copy link
Author

Choose a reason for hiding this comment

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

You are, of course, welcome to make any changes you desire.

@@ -167,15 +168,17 @@ export default {

afterEvent: function(chart, event) {

if (chart.config.options.scales.x.length == 0) {
return
const xScaleOptions = chart.config.options.scales.x || chart.config.options.scales.xAxes;

Choose a reason for hiding this comment

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

Same as above

@@ -278,6 +281,7 @@ export default {
},

resetZoom: function(chart) {
const xAxisName = chart.config.options.scales.x ? 'x' : chart.config.options.scales.xAxes ? 'xAxes' : undefined;

Choose a reason for hiding this comment

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

same as above, just take it from the options and if not specify fall back to default x


doZoom: function(chart, start, end) {
const xAxisName = chart.config.options.scales.x ? 'x' : chart.config.options.scales.xAxes ? 'xAxes' : undefined;

Choose a reason for hiding this comment

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

same as above

@ml054
Copy link

ml054 commented Jul 15, 2022

Any update on this? This prevents using your library with chart.js 3.5+, which means it effectively works with 3.4.x only.

@erunion
Copy link

erunion commented Aug 9, 2022

@AbelHeinsbroek any updates? Would love to get this pulled in.

@gregolsky
Copy link

Hi, any updates on this one?

@nategrift
Copy link
Collaborator

I will look into this

@nategrift nategrift added the bug Something isn't working label Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants