Check if the label on dataset level is needed

Joelius300 / ChartJSBlazor

This library is a modification of the awesome ChartJs.Blazor library by mariusmuntean. It's supposed to add more functionality to the LineChart and generally make the library more complete.

Other

40 stars 6 forks source link

Check if the label on dataset level is needed #46

Closed SeppPenner closed 4 years ago

SeppPenner commented 5 years ago

From https://github.com/Joelius300/ChartJSBlazor/issues/40#issuecomment-519690615:

We need to investigate whether the label on dataset basis is valid for all kind of charts.

For pie and doughnut, it has no effect: https://jsfiddle.net/g1anoety/ without the datalabels plugin enabled. (I would remove this for all charts where it's not yet needed and add it again if it's needed after we managed to get https://github.com/Joelius300/ChartJSBlazor/issues/34 done).

SeppPenner commented 5 years ago

Following https://www.chartjs.org/docs/latest/charts/:

line: Needed, check https://jsfiddle.net/ukwsn6c2/.
bar: Needed, check https://jsfiddle.net/ukwsn6c2/1/.
radar: Needed, check https://jsfiddle.net/ukwsn6c2/2/.
doughnut and pie: Not needed, check https://jsfiddle.net/g1anoety/.
polar area: Not needed, check https://jsfiddle.net/ukwsn6c2/3/. (We need to remove this!)
bubble: Needed, check https://jsfiddle.net/ukwsn6c2/4/.
scatter: Needed, check https://jsfiddle.net/ukwsn6c2/5/.

I have asked the guys from Chart.Js to check this as well and tell us, what's the idea behind this: https://github.com/chartjs/Chart.js/issues/6452.

Joelius300 commented 5 years ago

As already mentioned in https://github.com/Joelius300/ChartJSBlazor/issues/65 (https://github.com/Joelius300/ChartJSBlazor/pull/70), the wrong generateLabels function was hooked up for both pie- and polar-area- charts. Did that maybe influence your testing? If so, could you re-test and confirm the statements above so we're sure these aren't needed?

SeppPenner commented 5 years ago

I have tested this independently from our library using a JS fiddle (As you can see above in the links). So, there is no need to re-test it.

SeppPenner commented 4 years ago

After reading https://github.com/chartjs/Chart.js/blob/376da2131e8dcc10f3e9a7faae9f5f684a2402f2/src/index.js#L75: Can we assume that this will be only supported for a short time?

Joelius300 commented 4 years ago

What will only be supported for a short time? The datalabel plugin? We currently don't have support for this anyway so this shouldn't break anything once deprecated right?

SeppPenner commented 4 years ago

I don't understand their code or how it's structured. Is this for the datalabel plugin? I thought it was for the chart title^^

Joelius300 commented 4 years ago

But this issue is about labels on dataset level. What does this have to do with chart titles then? Does this really belong in this issue?

EDIT:

Is this about this title? If so, I don't think it belongs here.

SeppPenner commented 4 years ago

Ah, you're right. This was the label on dataset level, not the title on chart level.

SeppPenner commented 4 years ago

Will be tracked by https://github.com/mariusmuntean/ChartJs.Blazor/issues/43 after the merge.