Skip to content
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

Pie doughnut dataset weight #5951

Merged
merged 21 commits into from
Feb 27, 2019
Merged

Pie doughnut dataset weight #5951

merged 21 commits into from
Feb 27, 2019

Conversation

Vincent-Ip
Copy link
Contributor

@Vincent-Ip Vincent-Ip commented Jan 2, 2019

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.
screen shot 2019-01-01 at 11 52 09 pm

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.
@etimberg etimberg requested review from nagix and simonbrunel and removed request for nagix January 3, 2019 01:34
@Vincent-Ip
Copy link
Contributor Author

@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
@nagix
Copy link
Contributor

nagix commented Jan 4, 2019

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
Copy link
Member

simonbrunel commented Jan 4, 2019

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

simonbrunel commented Jan 9, 2019

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
kurkle previously approved these changes Feb 23, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

benmccann
benmccann previously approved these changes Feb 23, 2019
nagix
nagix previously approved these changes Feb 25, 2019
etimberg
etimberg previously approved these changes Feb 25, 2019
@etimberg etimberg added this to the Version 2.8 milestone Feb 25, 2019
@simonbrunel
Copy link
Member

@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 simonbrunel merged commit 93f4e6e into chartjs:master Feb 27, 2019
@simonbrunel
Copy link
Member

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

@Vincent-Ip
Copy link
Contributor Author

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

@sheixt
Copy link

sheixt commented Feb 28, 2019

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 😄

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants