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

Issue #1012. Added offsetAngle option for radar charts. #2984

Merged
merged 7 commits into from
Jul 19, 2016

Conversation

slinhart
Copy link
Contributor

Solution for #1012

Seems like with just a bit of refactoring to the polar chart's controller, this same thing could be applied to that chart as well.

I did not add any new tests for this, I think I will need some guidance on how best to test this.

@@ -98,6 +98,7 @@ scale | Object | [See Scales](#scales) and [Defaults for Radial Linear Scale](#s
*scale*.type | String |"radialLinear" | As defined in ["Radial Linear"](#scales-radial-linear-scale).
*elements*.line | Object | | Options for all line elements used on the chart, as defined in the global elements, duplicated here to show Radar chart specific defaults.
*elements.line*.lineTension | Number | 0 | Tension exhibited by lines when calculating splineCurve. Setting to 0 creates straight lines.
offsetAngle | Number | 0 | The number of degrees to rotate the chart clockwise.
Copy link
Member

Choose a reason for hiding this comment

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

In #2947 I called this startAngle, can we do the same here for consistency?

@etimberg
Copy link
Member

I'm not sure of the best way to test this. One option is to create the scale object then fake out the sizing done by the chart and test that angles returned from getDistanceFromCenterForValue are correct.

Another option is to create an entire radar chart (no need to fake out the sizing) and then test the scale functions correctly.

I quickly looked at the existing tests and it looks like we have one for the getDistanceFromCenterForValue. https://github.com/chartjs/Chart.js/blob/master/test/scale.radialLinear.tests.js#L390 Since that didn't break, that's a good sign.

You could also try gulp coverage then look at the generated html report to see which lines aren't covered. That's also a good starting point for writing a test.

CC: @tannerlinsley @simonbrunel @derekperkins @zachpanz88

…dded test to make sure correct angles are computed for all points in the radar chart (with and without startAngle option set).
@etimberg
Copy link
Member

+1 to merge now that tests are written

@panzarino
Copy link
Contributor

Agreed

@etimberg etimberg merged commit 88d9398 into chartjs:master Jul 19, 2016
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Issue chartjs#1012. Added offsetAngle option for radar charts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants