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 traces #3594

Merged
merged 26 commits into from
Mar 28, 2019
Merged

Sunburst traces #3594

merged 26 commits into from
Mar 28, 2019

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Mar 1, 2019

Sunburst traces are coming to plotly.js 🌞

peek 2019-03-01 15-10

1st iteration features

  • custom click iterations: clicking on branches zooms in, click on relative roots (in the middle) zooms out
  • per-sector coloring through marker.colors with per-branch color inheritance
  • optional per-sector values and two ways to sum them via branchvalues: 'total' | 'extra'
  • fully-featured hover, with hovertext, hoverinfo and hovertemplate support
  • pie-like sector coloring via sunburstcolorway and extendsunburstcolors
  • "leaf"-only opacity setting (to easily recreate common sunburst examples in the wild where leaves are less opaque then branches e.g. https://observablehq.com/@d3/d3-zoomable-sunburst)
  • the same auto-text-fitting algo used in pie traces

TODOs

  • agree on attribute names:
    • labels / parents / values or label / value / parent ?
    • level / maxdepth can anyone think of less-confusing names?
  • agree on branchvalues default 'total' or 'extra'?
  • experiment with storke-linejoin and stroke-miterlimit - see 1b21aae
  • make marker.line.width: 1 the default - done in c1ccad6
  • agree on click transitions easing and duration
  • fixup react + transitions edge cases
  • hook up uirevision done in: 0da13db
  • mucho jasmine tests
  • a few more mocks

Things that may be added in this PR

  • smooth transitions on click interactions + compatibility with Plotly.react transition API. I'll give this a try next week. THIS IS GOING IN --> aa3adf3
  • an enumerated marker.colorinheritance attribute to offer more ways to inherit sector colors
  • allow labels / parents duplication when unambiguous (d3-hierarchy is very strict)
  • replace d3-hierarchy (hierarchy, stratify, partition) calls with own implementation

cc @plotly/plotly_js @chriddyp @nicolaskruchten

- use d3-hierarchy (for now) to help out with the logic
- add special `plotly_sunburstclick` event to zoom in/out
- adapt pie's transformInsideText for sunbursts' ring
... for sunburst files when reading off the d3-hierarchy output
@etpinard etpinard added this to the v1.46.0 milestone Mar 1, 2019
@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Mar 4, 2019

This looks really nice!

One concern, re the text, which may also apply to pies... Is it possible to control the size so it's not 'auto'? Right now I worry about the larger text calling more attention to certain slices just because the text label is smaller therefore gets scaled up. Also aesthetically is it possible to just say "do a radial layout always even if text could be bigger/more horizontal"? Or possibly "do a symmetrical layout of text"? In certain cases, symmetry will be more highly-valued than absolute maximum readability...

@etpinard
Copy link
Contributor Author

etpinard commented Mar 4, 2019

Is it possible to control the size so it's not 'auto'?

yes, via insidetextfont.size

Also aesthetically is it possible to just say "do a radial layout always even if text could be bigger/more horizontal"?

Funny, a user wrote in about that in #3590 last week. I might try to ✅ in this PR.

@nicolaskruchten
Copy link
Contributor

yes, via insidetextfont.size

OK cool, does that disable the rotations or does it still try to get the most-horizontal-that-still-fits or... ?

@etpinard
Copy link
Contributor Author

etpinard commented Mar 4, 2019

does that disable the rotations

no, that's what #3590 requests.

... and use paper_bgcolor for marker.line.color dflt
@etpinard
Copy link
Contributor Author

etpinard commented Mar 6, 2019

First-cut transition work is on:

sunburst...sunburst-transitions

Here's an sample codepen: https://codepen.io/etpinard/pen/Bbpqvo,

I would appreciate some feedback. Thanks!

Edit: don't try clicking on a sector while a transition is happening, this part doesn't work at the moment. Thank you.

@antoinerg
Copy link
Contributor

antoinerg commented Mar 6, 2019

@etpinard Sunburst will be a strong addition to plotly.js 💪 and animations do add quite a bit of wow factor 🌟

Would it be possible/easy to have the labels appear after the resizing of the sectors is done. I'm asking because the labels are sometimes out of place during the transition. For example, when going from Aromas back to the root, you see the lines delimiting the sectors flying above the labels of the leaf nodes. This intermediate state, where labels are in between sectors and traversed by lines, is not correct or meaningful so I think we should hide it from the user.

@nicolaskruchten
Copy link
Contributor

WOW the sector tweens are gorgeous! nice job!!

That said, I agree with @antoinerg re the text. If we can translate the text with the sector tween, it should only start to appear after the sectors have stopped moving IMO.

@etpinard
Copy link
Contributor Author

etpinard commented Mar 7, 2019

Update with text-position transitions on sunburst...sunburst-transitions

examples --> https://codepen.io/etpinard/pen/LayPQQ

@antoinerg
Copy link
Contributor

examples --> https://codepen.io/etpinard/pen/LayPQQ

Text-position transition looks even better than I imagined 🎉 💪 Just superb!

@nicolaskruchten
Copy link
Contributor

OMG so awesome!

@chriddyp
Copy link
Member

chriddyp commented Mar 7, 2019

yeah that's gold 🏅

@jonmmease
Copy link
Contributor

So nice @etpinard!

what is double click intended to do on these? Seems to sometimes hide some of the wedges but I haven't been able to grok exactly what's going on.

On second try, it's not double clicking per-say, it's that clicking on the sunburst while it's animating sometimes causes some odd behavior with wedges disappearing. Maybe disable click interactions during transitions?

- get correct inside text contrasted color
- fix text position for concentric circle sector
- tweens sector paths and text 🎉
- first-cut Plotly.animate support
- calls Plotly.animate on sector click !
- disables sector clicks during transitions
- trigger plotly_click on root and leaf node
- clean up variable names and helpers, 1 routine to attach
  click and hover handlers !
@etpinard
Copy link
Contributor Author

etpinard commented Mar 8, 2019

Pre-vacation update

Updated example codepen: https://codepen.io/etpinard/pen/ZPybzB

I squashed all my transitions work into one large commit aa3adf3 where:

  • sector paths and text are tweened 🎉
  • sunburst will be our 2nd trace with support for transitions via Plotly.animate and Plotly.react
  • clicking on a sector simply calls Plotly.animate with the appropriate arguments.
  • the sector click handler is disabled during transitions, so the odd "wedges disappearing" behaviour is a thing of the past

The TODO list in the PR description has been updated. Please let me know if there's any other blocking items for this first iteration.

Now, I'm thinking of removing leaf.textposition and making sunburst traces only support inside text for all sectors except the hierarchy root. Getting outside text to tween properly will take at least 1 full day (see 92e5fe7): as most sunburst in the wild only have inside text, removing leaf.textposition sounds ok to me. Please let me know if you disagree. Update: 🔪 in 160bf4b

Moreover, I'm thinking of not implementing attributes sort, direction and rotation in this first iteration. Please let me know if that's ok. Update: 🔪 in 285b115

- to try to keep text inside sector, while
  keeping a transition smooth in the math sense.
- this would've been tricky to tween smoothly, most sunburst charts
  in the wild have text labels inside sectors only, so I didn't see
  the need to have this part of the first release.
... which could be moved back in if deemed necessary for
    the first iteration
@etpinard
Copy link
Contributor Author

Tagging this as reviewable cc @plotly/plotly_js

@antoinerg
Copy link
Contributor

agree on branchvalues default 'total' or 'extra'?

I'm still debating with myself about this one... I don't have a compelling argument for changing it from the current default extra to total. Maybe someone else does @plotly/plotly_js?

@antoinerg
Copy link
Contributor

This is one hell of a PR 🌞 ! Amazing work @etpinard 💪

💃 💃 💃

@etpinard etpinard merged commit 20553d5 into master Mar 28, 2019
@mwouts
Copy link

mwouts commented Apr 7, 2019

Thanks for the amazing contribution! I love the new sunburst plots! For those who can't wait for the next plotly.py release, have a look at the example below to see how to generate those plots in Python right now.

World population

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.

None yet

7 participants