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

Previously 0-values were not plotted into chart, now they do. #71

Merged
merged 1 commit into from
Nov 29, 2016

Conversation

cizambra
Copy link
Contributor

For ChartJS, data with 0 values were not being plotted into the chart, so there were gaps in places where it shouldn't. That leads to confusion, as the 0 values were confused with null values (a.k.a no data). An use case is for example a chart that measures the heartbeats of an application, 0 values NEED to be plotted as that data exists and is valuable.

@ankane
Copy link
Owner

ankane commented Nov 29, 2016

Hey, thanks @cizambra 👍 Looks like that was introduced in the latest release with this commit. 8071936

I believe the code should read (typeof rows[i][j] === "undefined") ? null : rows[i][j] and don't think we want the comment change since it'll be about a fixed bug.

Edit: Looks like rows[i][j] === undefined ? null : rows[i][j] is the convention used other places in the code (without quotes around undefined).

@ankane
Copy link
Owner

ankane commented Nov 29, 2016

Since this is a pretty big issue, going to push out a fix now.

@ankane ankane merged commit 691c63d into ankane:master Nov 29, 2016
@cizambra
Copy link
Contributor Author

Awesome, thank you for consider the change. I added the comment as it was my first pull request and I didn't know that the code can be reviewed later haha.

@ankane
Copy link
Owner

ankane commented Nov 30, 2016

This was a really big issue, so big thanks for not only being the first to report it but finding exactly what was going wrong and submitting a fix (this is more than 99% of people do). Not bad for a first PR :)

@cizambra
Copy link
Contributor Author

Thanks, if I can assist you with something else, just let me know ;).

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

2 participants