Add circular prop to arc element

[thabarbados]

Make Arc Elements depended on Circular prop. From issue #10357

You can check the build with changes here

[LeeLenaleee]

In its current implementation this would have to wait till version 4 since it will be a breaking change.
So for now I would suggest making it a configurable option.

[thabarbados]

Thank you for your answer @LeeLenaleee! How do you see the configurable option? For me, it looks like a new prop like circularArc: true|false, right?

[LeeLenaleee]

Yeah similar like how you can set the borderWidth, backgroundColor etc, make another option to make the arc flat or curved

[thabarbados]

@LeeLenaleee I've added a new Circular prop in the Arc element's option.
You can check the build with changes here.

[LeeLenaleee]

Looks good to me, only missing documentation about it: https://github.com/chartjs/Chart.js/blob/master/docs/charts/polar.md

Not sure if it also should be documented for pie/doughnut charts.
It gives a pretty interesting looking chart although the doughnut part does not work.
@etimberg what do you think, we also did not document the spacing for polarArea charts while it also technically works

[thabarbados]

Added a description to the docs

[LeeLenaleee]

Dont think the typing need an update but need to check that at home

[stockiNail]

@thabarbados @LeeLenaleee sorry if I jump in the thread. I think the elements doc should be updated accordingly with new option. Am I wrong?

https://github.com/chartjs/Chart.js/blob/master/docs/configuration/elements.md

[thabarbados]

@stockiNail, I think you are right. I added a description to this section of the docs.

[stockiNail]

@thabarbados apologize again... the curcular option must be but in the stylying section where is reported that the fallback is to arc element

[thabarbados]

@stockiNail Sorry, I don't understand. The description of circular option should be in the Arc Styles section, like a description of the pointStyle here?

[stockiNail]

@stockiNail Sorry, I don't understand. The description of circular option should be in the Arc Styles section, like a description of the pointStyle here?

I meant, in the polar.md, row 69, you have the circular option which is linked to general section. In my opinion should be linked to styling section.

In PR:

| [`circular`](#general) | `boolean` | Yes | Yes | `true`

should be

| [`circular`](#styling) | `boolean` | Yes | Yes | `true`

because in styling section is written that

All these values, if `undefined`, fallback to the associated [`elements.arc.*`](../configuration/elements.md#arc-configuration) options.

and this should be the behavior of circular option when missing.

[stockiNail]

And the row 78 must be moved in row 90

[thabarbados]

@stockiNail Thank you very much for your review. I have fixed these errors.

[etimberg]

Code looks fine to me. Not sure if this would benefit from an image based test or not

[thabarbados]

@LeeLenaleee @etimberg thanks for your approves. Can you help me and tell me what else I need to do to merge PR?

[LeeLenaleee]

We need to wait with merging before we are sure we want to release a version 3.9.0

If we merge it now we can't do a bug fix release (3.8.x) anymore. So when there are no plans for a bugfix release all the enhancements for 3.9 will get merged into master and bundled in the next release.

[thabarbados]

Thanks a lot for the answer. I understand your workflow now :)

[etimberg]

@kurkle do you want to review this at all?

[kurkle]

This one is hard to review on phone. I can give it a proper look tomorrow, but I'm quite sure I'll have nothing to comment. So its ok if you want to go ahead and merge.

[kurkle]

The test is quite useless (I think it would pass without the code changes). An image based test would be better.