Pie doughnut dataset weight #5951

Vincent-Ip · 2019-01-02T05:18:22Z

I thought that this could help other people trying to adjust the relative size of Pie and Doughnut datasets. I saw a feature request for something similar #4724 and I also had a need to vary the thickness to make the pie chart look they way I wanted.

Here's an example of what the chart looks like with the new dataset attribute - weight.

In the above example, the inner dataset was given a weight of 3. This results in a data set that is 3 times the size of outer dataset (which doesn't specify a weight and so has a default weight of 1).

Fixes #4724
Fixes #5330
Fixes #5612

Weight affects the relative thickness of the dataset when there are multiple datasets in pie & doughnut charts. The default weight of each dataset is 1. Providing any other numerical value will allow the pie or doughnut dataset to be drawn with a thickness relative to its default size. For example a weight of 2 will allow the dataset to be drawn double its typical dataset thickness. NOTE: The weight attribute will only have an affect on a pie or doughnut chart if there is more than one visible dataset. Using weight on a pie or doughnut dataset when there is only one dataset on the chart will have no affect.

src/controllers/controller.doughnut.js

src/core/core.controller.js

src/controllers/controller.doughnut.js

Vincent-Ip · 2019-01-04T00:12:29Z

@benmccann Thanks for the feedback. I'll make those changes.

- added back getRingIndex function to doughnut controller. - moved getVisibleDatasetTotalWeight from the base controller file to the doughnut controller. - removed parentheses from the chartWeight assignment statement. - Also, slightly renamed the new functions

src/controllers/controller.doughnut.js

simplicfications.

src/controllers/controller.doughnut.js

nagix · 2019-01-04T11:23:26Z

The weight values in datasets are referenced many times for a single draw call, so there is the same issue as the comment in #5841. I'm wondering if we can cache weight for each dataset and the total weight value when the first visible dataset is updated.

simonbrunel · 2019-01-04T12:14:45Z

Good point @nagix but it may be better to tackle this optimization in a separate PR (maybe by registering an internal plugin computing these values during beforeDatasetsUpdate).

I like this feature, however we may want a different API for it. I was already thinking to use weight to control the dataset order (many time requested) which I think matches better. But I also remember requests to control the radius so we should think this API to support this case without duplicating options.

Highchart uses size: '42%':

size: 42 would generate a ring with a size of 42px
size: "75%" would be similar to this PR weight: 3

If we really want to implement the weight approach, maybe we could use the flex name (from CSS). But in this case, I'm not sure how it will work along the size (or whatever future option) controlling the size of the ring.

simonbrunel · 2019-01-04T13:05:50Z

I'm not sure how it will work along the size (or whatever future option) controlling the size of the ring.

flex could be ignored from all computations if size is explicitly defined. So I'm fine with both: flex (this PR) or size as percentage, however flex would be easier as a first step and already almost implemented.

I would definitely reserve weight to control the dataset order, which applies to all type of charts. Also note that we already use weight to sort scales, so would be more consistent. flex is pretty common, especially for CSS developers. Other suggestions for the option name are of course welcome :)

Thoughts?

Vincent-Ip · 2019-01-09T17:02:24Z

Hi @simonbrunel , sorry for the slow reply. I thought that perhaps you were asking the question of your co-reviewers. I do agree with you that this parameter should be called something other than weight, since you are using weight in other places of something different.

flex seems like a possible fit, but the css usage of flex itself seems different (certainly at least in syntax). Perhaps without knowing what the future plans for size and chart resizing behavior, i'm at a bit of a disadvantage for seeing how close this feature fits with the css usage of flex and how closely (or not), this feature would resemble it.

i'm happy to change it to flex, but i'm not sure it won't be confusing. It will certainly be less confusing than weight. In terms of other possible names for this feature, nothing exceptional comes to mind. Perhaps it could be named something more explicit like relative-size, or ring-scale?

Sorry to give you back a question, but... what do you think?

simonbrunel · 2019-01-09T18:07:10Z

Thanks @Vincent-Ip

I would go for a name that will also work for other charts, for example we could implement it to control the size of bars when side by side (which is of course not the purpose of this PR). That's why I would eliminate ring-scale. I would also discard relative-size because a size to me is 64px, 50%, 15rem but not a weight.

We are not designing our features to support exactly the same syntax as CSS, I'm just saying that flex is a well-know CSS feature that represents exactly what this PR is trying to implement.

For example (CSS)

.col-1 { flex: 2; }  // size: 25%
.col-2 { flex: 2; }  // size: 25%
.col-3 { flex: 4; }  // size: 50%

In this case, it's the same syntax and the same behavior, so could be a good match.

kurkle

Looks good to me.

simonbrunel · 2019-02-27T14:53:25Z

@Vincent-Ip I think we should remove the pixel based test since it's exactly the same as the image based ones (see this comment).

simonbrunel · 2019-02-27T22:07:54Z

Thanks @Vincent-Ip for this work and all the iterations.

Vincent-Ip · 2019-02-27T22:12:25Z

@simonbrunel No problem. Thank you and all other reviewers for the experience.

sheixt · 2019-02-28T11:16:50Z

Ah looks like I missed the progression of this, was only following #4724. Looks great guys, I'll try to find the time to give it a spin and adjust my workaround 😄

Add functionality to give pie & doughnut datasets a weight attribute, which affects the relative thickness of the dataset when there are multiple datasets in pie & doughnut charts. The default weight of each dataset is 1, providing any other numerical value will allow the pie or doughnut dataset to be drawn with a thickness relative to its default size. For example a weight of 2 will allow the dataset to be drawn double its typical dataset thickness. Note that the weight attribute will only affect a pie or doughnut chart if there is more than one visible dataset. Using weight on a pie or doughnut dataset when there is only one dataset on the chart will have no affect.

Vincent-Ip added 3 commits January 1, 2019 23:18

Updated the doughnut documentation

1c2f11e

trying to reduce the Cognitive Complexity of getRingWeightOffset

a977196

etimberg requested review from nagix and simonbrunel and removed request for nagix January 3, 2019 01:34

benmccann reviewed Jan 3, 2019

View reviewed changes