Allow specifying bar chart via xy data points#4565
Allow specifying bar chart via xy data points#4565simonbrunel merged 1 commit intochartjs:masterfrom
Conversation
benmccann
changed the title
src/controllers/controller.bar.js
Outdated
| }); | ||
|
|
||
| function getValue(scale, dataPoint) { | ||
| return typeof dataPoint === 'string' ? Number(dataPoint) : scale.getRightValue(dataPoint); |
There was a problem hiding this comment.
I wonder if this logic should be in scale.getRightValue or not
There was a problem hiding this comment.
I tried that and it seemed to break the tests. I also think it's rather silly that people would pass in a number as a string and would just as much consider deprecating support for that in 3.0 so wasn't sure I wanted to encourage to be done for even more scales
There was a problem hiding this comment.
I agree with @etimberg, would be better in getRightValue() but only for the linearbase impl. (as we did with the time scale). I don't think it's silly because +'1.2', Number('1.2') or parseFloat('1.2') return valid numbers. Linear scales expect numbers from the data, so as long as the values represent valid numbers, I'm not sure why we would want to make it less flexible (we don't know from where the user gets the data, could be from an API that returns numbers as string).
There was a problem hiding this comment.
I'm not sure how to do that. If I add the following to scale.linearbase.js then all the tests start failing:
getRightValue: function() {
return Chart.Scale.prototype.getRightValue.call(this);
},
There was a problem hiding this comment.
getRightValue: function(value) {
if (typeof value === 'string') {
return +value;
}
return Chart.Scale.prototype.getRightValue.call(this, value);
}There was a problem hiding this comment.
thank you! i feel dumb now. too early after waking up I guess...
I've updated the PR with this change
|
|
||
| for (; i < ilen; ++i) { | ||
| d = dataset.data[i]; | ||
| if (d !== null && d !== undefined && !isNaN(d)) { |
There was a problem hiding this comment.
What's the reason of this change?
There was a problem hiding this comment.
Because isNaN returns true when you pass in an object
Also, I don't know if it's really the bar controller's responsibility to do such a check anyway. It shouldn't need to know the logic for whether the rectangle element can be drawn. That logic should be encapsulated in the rectangle element.
There was a problem hiding this comment.
Looks like a regression, null/undefined/NaN data are currently used to skip element (@etimberg?) Should be the same logic as your other change: d = scale.getRightValue(dataset.data[i]).
I doubt that's the responsability of the element to decide whether it needs to drawn itself. The controller seems a better candidate, each controller can have different behavior.
There was a problem hiding this comment.
I think the current behaviour for bars is that null/undefined/NaN are supposed to just not draw anything but I'm not 100% sure on that. I know we use those values to skip drawing grid lines in the axis though
There was a problem hiding this comment.
Yes, I like scale.getRightValue much better than the old check. I've updated it to that
|
Is this feature functional? The 'bar' chart specified by xy in my code are not plotting. newbie to chart.js here. Really appreciate this beautiful and easy-to-use library. |
|
I believe it works unless you skip data points. There is currently an improvement pending for that: #5298 |
4 participants
Fixes #2892