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 some ES6 syntax issue and add support for getting axis keys from options #85

Merged

Conversation

elesueur
Copy link
Contributor

xAxis and yAxis keys are currently hard-coded as either 'x', or 't' and 'y'.

This change adds support to use the provided xAxisKey and yAxisKey values from the chartjs options object that is provided by the user.

Fall back to the original behavior in case these are not provided.

This is a non-breaking change.

Also, add the 'const' keyword in front of the addFilter method.

…options

xAxis and yAxis keys are currently hard-coded as either 'x', or 't' and
'y'.

This change adds support to use the provided xAxisKey and yAxisKey
values from the chartjs options object that is provided by the user.

Fall back to the original behavior in case these are not provided.

This is a non-breaking change.

Also, add the 'const' keyword in front of the addFilter method.
@elesueur elesueur force-pushed the topic/elesueur/fixes-and-axis-key-configs branch from 813bc61 to c9fd2fb Compare October 13, 2023 17:02
@Makanz Makanz merged commit e16c9d4 into Makanz:main Jan 22, 2024
Comment on lines +58 to +59
const xAxisKey = dataset.trendlineLinear.xAxisKey || parsing ? parsing.xAxisKey : "x";
const yAxisKey = dataset.trendlineLinear.yAxisKey || parsing ? parsing.yAxisKey : "y";
Copy link

@DialaHamadneh DialaHamadneh Jun 23, 2024

Choose a reason for hiding this comment

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

Hi @Makanz, I believe this line won't work as intended if dataset.trendlineLinear.xAxisKey has value, it'll throw Cannot read properties of undefined
these lines should be 👇🏼

const xAxisKey = dataset.trendlineLinear.xAxisKey || (parsing ? parsing.xAxisKey : "x");
const yAxisKey = dataset.trendlineLinear.yAxisKey || (parsing ? parsing.yAxisKey : "y");

Copy link
Owner

Choose a reason for hiding this comment

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

Please make a pull request if you have the possibility. Else I will try to take a look at it in a couple of weeks.

Copy link
Owner

Choose a reason for hiding this comment

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

@DialaHamadneh I have released a new version were I have made the change.
https://www.npmjs.com/package/chartjs-plugin-trendline/v/2.1.1

Copy link

@DialaHamadneh DialaHamadneh Jun 25, 2024

Choose a reason for hiding this comment

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

Thanks @Makanz , it's working 🎉 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants