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

Add circular prop to arc element #10405

Merged
merged 7 commits into from
Jul 30, 2022

Conversation

thabarbados
Copy link
Contributor

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

You can check the build with changes here

@LeeLenaleee
Copy link
Collaborator

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

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

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

@thabarbados
Copy link
Contributor Author

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

Copy link
Collaborator

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

Added a description to the docs

Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

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

docs/charts/polar.md Outdated Show resolved Hide resolved
docs/charts/polar.md Outdated Show resolved Hide resolved
LeeLenaleee
LeeLenaleee previously approved these changes Jun 8, 2022
@stockiNail
Copy link
Contributor

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

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

@stockiNail
Copy link
Contributor

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

@thabarbados
Copy link
Contributor Author

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

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

And the row 78 must be moved in row 90

@thabarbados
Copy link
Contributor Author

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

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

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

@thabarbados
Copy link
Contributor Author

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

@LeeLenaleee
Copy link
Collaborator

LeeLenaleee commented Jun 14, 2022

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

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

@etimberg
Copy link
Member

@kurkle do you want to review this at all?

@kurkle
Copy link
Member

kurkle commented Jul 27, 2022

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.

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.

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

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.

5 participants