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

Fix: Arc getCenterPoint when full circle #9152

Merged
merged 1 commit into from May 24, 2021
Merged

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 24, 2021

Fix: #9151

@kurkle kurkle requested a review from simonbrunel May 24, 2021 20:12
@etimberg etimberg added this to the Version 3.3.1 milestone May 24, 2021
@etimberg etimberg merged commit 1d04735 into chartjs:master May 24, 2021
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

A bit late since it's already merged but I don't think this fix is enough: the datalabels plugin don't use this method but startAngle and endAngle. I didn't investigate this issue but I feel that endAngle is % 360 somewhere for some internal calculations while it should not be modified on the instance.

@kurkle
Copy link
Member Author

kurkle commented May 24, 2021

Thanks @simonbrunel, I agree its not enough when those properties are used. So the fix is a bit more complex.

@simonbrunel
Copy link
Member

All the element.endAngle mutations in the draw* methods look suspect (e.g L172) 🤔

Not sure the draw methods should change the element state.

@kurkle
Copy link
Member Author

kurkle commented May 25, 2021

Agreed, it should use something internal when it needs to split the drawing in passes. And it really does not need many passes in this case.

@kurkle kurkle deleted the full-circle branch May 25, 2021 04:35
kurkle added a commit to kurkle/Chart.js that referenced this pull request May 25, 2021
etimberg pushed a commit that referenced this pull request May 25, 2021
* Stop mutating arc state while drawing

* No need for default values

* Nits

* Remove #9152

* Use correct endAngle for clipping
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.

Label incorrectly positioned with doghnut graph
3 participants