v2 use of dataset.type==='bar' tests in controller.bar.js

[xriss]

Hi guys,

I'm trying to use the v2 dev and build some custom graphs on top of it so expect a few related issues :)

One of the problems I've found is trying to do some simple extensions to the bar graph, I just wanted to replace the draw function with one that also draws the numeric value on top of the bar.

Easy enough to do but since I've created a "bartop" controller and am simply reusing all of the bar controller code the explicit tests for dataset.type are failing since the value is not "bar" any more, eg.

https://github.com/nnnick/Chart.js/blob/v2.0-dev/src/controllers/controller.bar.js#L39

Are these tests really necessary? I'm not sure why they are needed except to catch old datasets or auto filter out only some datasets to draw when multiple ones are present?

Maybe something that would be better served explicitly with just the isDatasetVisible test and manual switching of datasets on and off by the user.

Obviously I can easily work around all of this and of course I have. Still I'm suggesting that this hard coded value test should be removed to make this sort of simple overloading (something you expect people to do?) work easier out of the box.

[etimberg]

@xriss the reason for those tests is that you could have a mixed chart containing lines & bars. You want to know the number of bars so that the width of each bar can be calculated correctly.

I'll see if I can think of something to make this nicer.

[xriss]

I'm still trying to understand the code but possibly not setting the dataset.type automatically when you load up the data?

Then the test there could just be for "bar" or undefined and pick up any dataset that isn't explicitly set to something else.

Looks like this would involve dealing with dataset.type being undefined in some bits of the core code.

[etimberg]

I like the idea of just checking dataset.bar. It could be added alongside the current dataset.type field since the type is needed to find the correct controller constructor. Then any controller that wants to be treated as a bar could simply add the bar parameter to their dataset.

[etimberg]

@xriss what do you think of #1788?

[xriss]

Thank you, that looks great to me.

although I think the test at

https://github.com/nnnick/Chart.js/blob/v2.0-dev/src/controllers/controller.bar.js#L219

needs the same small update as well.

BTW

Here's a gist of what I currently have to simply take the bar chart and replace the draw function to put the value on top of each bar.

https://gist.github.com/xriss/640126818e3320733622

[etimberg]

Good call, I will update that tonight and get this merged 😄

Your gist looks good. If you have the latest copy of the v2.0-dev branch you could also do

Chart.controllers.bartop = Chart.controllers.bar.extend({
    draw: // your draw function here
});

If that doesn't work, we've screwed up somewhere and I can look into it.

[etimberg]

Closing as I've updated & merged #1788