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

Added property outerArcLength to Sunburst #7707

Merged
merged 2 commits into from
Jan 26, 2018

Conversation

jon-a-nygaard
Copy link
Contributor

Description

Added the property outerArcLength to use as an alternative filter instead of innerArcLength.
In the case of points where innerR is 0, the innerArcLength will also be 0, which may cause unwanted behaviour.
It can be seen in our current Sunburst demo that the data label in the top node is filtered away, even though there is enough space for the data label.

The sample highcharts/demo/sunburst is modified to use outerArcLength on level 1, but it is depedent upon the changes in #7680 for level.levelIsConstant to work properly.

Note: I removed innerArcFraction and perimeter, because I found that innerArcLength could be expressed as radians * values.innerR.

Example(s)

Related issue(s)

@oysteinmoseng
Copy link
Member

Probably good, just wondering if we should avoid the magic number 6.28? Assuming this is 2*PI, could we just store that in a variable since we are using that anyway? Or at least add a comment on how this works

@jon-a-nygaard
Copy link
Contributor Author

jon-a-nygaard commented Jan 25, 2018

Thanks @oysteinmoseng! I agree that we should avoid magic numbers.

@jon-a-nygaard
Copy link
Contributor Author

Closed by accident

@jon-a-nygaard jon-a-nygaard reopened this Jan 25, 2018
@TorsteinHonsi TorsteinHonsi merged commit f906aa9 into master Jan 26, 2018
@jon-a-nygaard jon-a-nygaard deleted the feature/sunburst-property-outerarclength branch February 11, 2019 09:02
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.

3 participants