Skip to content

Conversation

@etimberg
Copy link
Member

@etimberg etimberg commented Dec 31, 2019

Adds a new option, radiusPercentage that shrinks the outside radius of doughnut / pie charts. A percentage is used to enable more flexibility during responsive conditions.

Closes: #3923

Adds a new option, `radiusPercentage` that shrinks the outside radius of
doughnut / pie charts. A percentage is used to enable more flexibility
during responsive conditions.
@etimberg etimberg added this to the Version 3.0 milestone Dec 31, 2019
@benmccann
Copy link
Contributor

benmccann commented Dec 31, 2019

Maybe this should be called padding to be more generic and so it could eventually be made available to all chart types?

@kurkle
Copy link
Member

kurkle commented Jan 1, 2020

I'm not sure I get the use case. layout.padding could be used, but I don't think that supports percentages.
If planning on having mutiple pies in one canvas, we should debate if having a chartArea per dataset and handling that in layout would be better?

@etimberg
Copy link
Member Author

etimberg commented Jan 1, 2020

I don't think multiple pies per canvas is something I'd want to do. It seems like it'd be a lot more complex for a few small use cases.

I could make this into a padding option. It'd be inside the chart area only. From the original issue, it was unclear if users wanted pixels or percentages. I went with a percentage to match the cutoutPercentage that already exists for pie/doughnut charts

@kurkle kurkle changed the title Allow control of the doughnut chart radius - #3923 Allow control of the doughnut chart radius Jan 3, 2020
@etimberg etimberg requested a review from benmccann January 5, 2020 19:54
@benmccann
Copy link
Contributor

benmccann commented Jan 6, 2020

I think if we made this a padding option it'd be fine to support percentages. Ideally we'd support percentages and pixels, but that wouldn't have to be done immediately

@benmccann
Copy link
Contributor

This looks fine to me other than maybe the option name. What did you guys think about it?

@kurkle
Copy link
Member

kurkle commented Jan 14, 2020

I too dislike having 'percentage' in the option name. Both options should be changed to be consistent through. One option would be to use only cutout and radius, requiring string with % for percentage and assume number are pixels. But I think that is out of the scope of this PR.

@benmccann
Copy link
Contributor

Yeah, keeping "percentage" out of the name is a good point too. My main thought was to call it padding so that the same name could at some point be used for other chart types

@kurkle
Copy link
Member

kurkle commented Feb 24, 2020

This looks fine to me other than maybe the option name. What did you guys think about it?

The name is consistent with other options currently, so if we want to change those then its can be done in another PR. This should be merged, IMO.

@benmccann
Copy link
Contributor

I don't have strong feeling about it. If you'd like to merge that's okay with me. It will need to be rebased though

@etimberg
Copy link
Member Author

etimberg commented Mar 8, 2020

Going to close this. I'm sure there's a better way to achieve this

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.

Pie/Dougnut radius

3 participants