-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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 bar chart with {x, y} data points #4673
Conversation
nagix
commented
Aug 17, 2017
•
edited
Loading
edited
- Use the value scale to check the value when drawing bars
- Fix a document issue
- This fixes [BUG] Bars with {x, y} data points are not drawn on a time scale #4661.
- Use a value scale to check the value when drawing bars - Add null check in arrayUnique() - Fix document issue
src/scales/scale.time.js
Outdated
@@ -71,7 +71,7 @@ function arrayUnique(items) { | |||
|
|||
for (i = 0, ilen = items.length; i < ilen; ++i) { | |||
item = items[i]; | |||
if (!hash[item]) { | |||
if (item !== null && !hash[item]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then that's not anymore arrayUnique
if it filters null
values. Can we make that check elsewhere without iterating the whole array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can filter null out when the timestamps
array is built, but not for the labels
array because it is copied to datasets
later.
Maybe we can do this way.
labels = arrayUnique(labels).sort(sorter);
if (labels[0] === null) {
labels.shift();
}
timestamps = arrayUnique(timestamps).sort(sorter);
if (timestamps[0] === null) {
timestamps.shift();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it doesn't work because a timestamp can be a negative value. We need to prepare both labels
that doesn't contain null and rawLabels
that can contain null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
negative timestamp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
negative timestamp would be before 1-1-1970
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the null check to the outside of arrayUnique
.
src/scales/scale.time.js
Outdated
var i, j, ilen, jlen, data, timestamp; | ||
|
||
// Convert labels to timestamps | ||
for (i = 0, ilen = chart.data.labels.length; i < ilen; ++i) { | ||
labels.push(parse(chart.data.labels[i], me)); | ||
timestamp = parse(chart.data.labels[i], me); | ||
if (timestamp !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etimberg do we support null
in the data.labels
array (what use case)? If not, I would consider that as an error and would avoid duplicating the labels
timestamps array with rawLabels
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think null
should be in data.labels
@nagix are you sure we are actually checking the correct cases? I think we just want to skip labels: ['2017', null, '2018'] // INVALID (what does that mean?)
data: [{x: null, y: 42}] // INVALID (what does that mean?)
data: [42, null, 16] // VALID (skip the second value mapped to the second label) |
@simonbrunel Actually it is intended to check if timestamps are correctly parsed by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to not gracefully handle invalid timestamps and have the user to provide correct inputs to the chart. Only explicitly null
data must be ignored, everything else should be considered invalid user inputs generating invalid chart.
Agreed. I removed null timestamp checks. |
Can we merge this then? |