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

Sunburst #907

Closed
wants to merge 7 commits into from
Closed

Sunburst #907

wants to merge 7 commits into from

Conversation

blairn
Copy link
Contributor

@blairn blairn commented Apr 8, 2015

I am not good at git, but... I think this is right.
It works, tests, lints. I'll do more clean up in the next few days, along with more charts being pull requested.

Enjoy!

@gordonwoodhull
Copy link
Contributor

Examples are great, but we will need Jasmine tests as well. Please see if you can exercise the functionality with some tests similar to https://github.com/dc-js/dc.js/blob/develop/spec/pie-chart-spec.js

@blairn
Copy link
Contributor Author

blairn commented Apr 8, 2015

I thought I did have tests o.O. I remember writing them, I'll go have a
look now

On Wed, Apr 8, 2015 at 9:12 PM, Gordon Woodhull notifications@github.com
wrote:

Examples are great, but we will need Jasmine tests as well. Please see if
you can exercise the functionality with some tests similar to
https://github.com/dc-js/dc.js/blob/develop/spec/pie-chart-spec.js


Reply to this email directly or view it on GitHub
#907 (comment).

@blairn
Copy link
Contributor Author

blairn commented Apr 8, 2015

ah, I do have tests, but forgot to commit them :( - me and git have a
difficult arrangement.

How do you want me to add them? commit it and build another pull request
replacing the one I did before? or send it to you so you can commit it in?

On Wed, Apr 8, 2015 at 9:16 PM, Blair Nilsson blair.nilsson@gmail.com
wrote:

I thought I did have tests o.O. I remember writing them, I'll go have a
look now

On Wed, Apr 8, 2015 at 9:12 PM, Gordon Woodhull notifications@github.com
wrote:

Examples are great, but we will need Jasmine tests as well. Please see if
you can exercise the functionality with some tests similar to
https://github.com/dc-js/dc.js/blob/develop/spec/pie-chart-spec.js


Reply to this email directly or view it on GitHub
#907 (comment).

@blairn
Copy link
Contributor Author

blairn commented Apr 8, 2015

ah... my tests fail LINT something awful, I'll work on getting them working
^.^ and make a new PR. Good night, and thanks for everything! dc.js
wouldn't be nearly as wonderful without you!

On Wed, Apr 8, 2015 at 9:19 PM, Blair Nilsson blair.nilsson@gmail.com
wrote:

ah, I do have tests, but forgot to commit them :( - me and git have a
difficult arrangement.

How do you want me to add them? commit it and build another pull request
replacing the one I did before? or send it to you so you can commit it in?

On Wed, Apr 8, 2015 at 9:16 PM, Blair Nilsson blair.nilsson@gmail.com
wrote:

I thought I did have tests o.O. I remember writing them, I'll go have a
look now

On Wed, Apr 8, 2015 at 9:12 PM, Gordon Woodhull <notifications@github.com

wrote:

Examples are great, but we will need Jasmine tests as well. Please see
if you can exercise the functionality with some tests similar to
https://github.com/dc-js/dc.js/blob/develop/spec/pie-chart-spec.js


Reply to this email directly or view it on GitHub
#907 (comment).

@gordonwoodhull
Copy link
Contributor

Yes, adding them to the same branch and pushing them is the right approach. A PR is always a work in progress. Lmk if you get stuck and I'll help troubleshoot. The tests are often pretty challenging.

@KatiRG
Copy link

KatiRG commented Jun 30, 2015

I was able to get the sunburst chart working using the code here https://github.com/blairn/dc.js/tree/sunburst, but the weird this is that it turns all my rowChart bars dark grey. Anyone else find this?

@KatiRG
Copy link

KatiRG commented Jun 30, 2015

for now we have just hard-coded it in the css:

.dc-chart rect { fill: #1f77b4; }

@gordonwoodhull
Copy link
Contributor

@csymill26 asked in #1069 and #781:

The sunburst is an awesome graph. Is there a way to make it zoomable? I have not been able to make it do so. Also, .innerRadius does not seem to work for me either. No matter the value I give it, the inner ring is the same size. I saw someone else mentioned having this problem. Is there a fix? Another thing, is it possible to add spaces between the levels? Thanks!

@gordonwoodhull
Copy link
Contributor

@csymill26, would you expect the sunburst also to "filter in" to the zoomed part, or is it just a view thing. Also what would be the interface, something like single click to filter, double click to zoom?

@csymill26
Copy link

Zooming in would be a visual thing as well as a filter thing. It doesn't matter to me if it's a single or double click to zoom.

@csymill26
Copy link

Couple questions: Is the inner radius value working yet? Will you add zooming? If it/they are not working yet, when will these become available? Thank you!

@gordonwoodhull
Copy link
Contributor

Unfortunately I think @blairn is quite busy, since he hasn't gotten back to this PR for a while. I'd be willing to write the tests if someone else wants to implement these features.

@csymill26, probably your best bet if you need this stuff immediately is to start from Blair's source and make the changes yourself. Then please file another PR with your updates!

@gordonwoodhull
Copy link
Contributor

Oops, I missed the tests being added. That's awesome. I guess it is on me to get them running properly.

@staveris
Copy link

Hi regarding innerRadius, i noticed that in dc.js line 4548 here inner and outerradius are set as functions of d.y and d.dy. I replaced them with .innerRadius(_innerRadius) and it works. take care when changing though since it will make your graph look odd.

@gordonwoodhull
Copy link
Contributor

Merged via #1388 for 3.0.0

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.

5 participants