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

Fixed misplaced data points on category scale #4062

Merged
merged 5 commits into from
Mar 27, 2017

Conversation

martinzuern
Copy link
Contributor

Verifying the index before processing getPixelForValue
See #4060

if (value !== undefined && isNaN(index)) {
// If value is a data object, then index is the index in the data array,
// not the index of the scale. We need to change that.
var valueCategory = me.isHorizontal() ? value.x : value.y;
Copy link
Member

Choose a reason for hiding this comment

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

this will error if value is undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right, thank you for your feedback. Addressed this on the latest commit.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

It fails because value can also be null

@martinzuern
Copy link
Contributor Author

@simonbrunel But that would be catched by line 77, right?

@simonbrunel
Copy link
Member

  • line 72 rejects only undefined value but not null
  • line 73 tries to access value.x or value.y, but value can be null

@martinzuern
Copy link
Contributor Author

@simonbrunel Please review

@etimberg etimberg merged commit 6f99157 into chartjs:master Mar 27, 2017
@simonbrunel simonbrunel added this to the Version 2.6 milestone May 7, 2017
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.

3 participants