Icicle Trace

[Kully]

closes: #5369

Overview of the Pull Request:

This PR is for the purposes of adding an Icicle/Flame charts to the Plotly traces. The high level aim is for the Icicle to achieve parity with the sunburst and treemap traces, and other features

• Original Proposal GIST -> here
• Reference Icicle branch (author: @archmoj) -> here (do not modify)

Setting up the PR

• git rebase their local branch off the latest master,
• make sure to not git add the dist/ folder (the dist/ is updated only on version bumps),
• make sure to commit changes to the package-lock.json file (if any new dependency required),
• write an overview of what the PR attempts to do,
• select the Allow edits from maintainers option (see this article for more details).

List of Tasks

• Working Prototype of Icicle Plot (see )
• Pathbar (from Treemap)
• Max Depth Functionality
• Include mocks/baselines of every treemap variant
• Achieve parity with sunburst and treemap traces
• Add mocks/baselines for Icicle-specific features
• Directions for all Orientations of Icicle (up, down, left, right)
• Modify leaf-opacity mock by reversing order of four icicles
• Ensure icicle fills up domain if maxdepth used (horizontal/vertical)
• Jasmine tests
• Bug: No transition for incoming text
  • On transitioning 'up' the tree, the text appears in its new location rather than transitioning in
• Bug: Clicking Root Node Causes Sweep Animation
  • Icicle root expands to fill entire domain if clicked. Use "root: {"color": "red"} to see the bug (or another color).

Icicle.Bug.-.Root.is.Clickable.mov

[Kully]

Bug - No transition on slicetext

While playing around with the Icicle chart, I found a bug:

1. Ensure that Icicle has transition enabled
2. Click on a slice or pathbar so as to pan the camera to reveal slices that are not in the layout.
3. Finally, notice that the slicetrace is already fixed into place and they do not transition.

icicle.-.text.no.transition.mov

[Notice that the Awan slice is the only one that has no transition. It's also the only one that has children. All the other slices seem to behave normally]

Looking deeper into this, I found that this behaviour is most likely a by-product of the way the transition logic is coded for all of Sunburst, Treemap, and now Icicle:

Sunburst: Notice how Thurs comes into view not by stretching and sheering, but is already where it needs to be, with scale being the main param for its transition.

sunburst_2.mov

Insights/Resolutions

I was exploring the makeUpdateTextInterpolator function but was not able to find anything relevant.

Do any of you have insights into what the reason/problem could be?

CC
@nicolaskruchten @alexcjohnson

Maybe the path of least resistance is to only make text visible once the slides animate into place, kinda like how treemap works sometimes:

treemap.-.text.appears.mov

[nicolaskruchten]

Re the transition bug, if it already impacts treemap/sunburst, let's just leave it as-is here and log an issue to cover the existing ones and then loop back around once this PR is merged to tackle it for all three trace types.

[Kully]

Re the transition bug, if it already impacts treemap/sunburst, let's just leave it as-is here and log an issue to cover the existing ones and then loop back around once this PR is merged to tackle it for all three trace types.

That sounds like a solid plan. Will do that.

[archmoj]

Instead of adding 4 mocks added 1477ceb, can't we add a simple & single mock to test 4 directions?
You also need to generate baselines (see here) for new mocks and validate them in test/jasmine/tests/mock_test.js.
I also suggest you start test/jasmine/tests/icicle_test.js and copy and adapt good portion of treemap and sunburst tests, e.g. supply defaults, calc and other tests sooner than later.
Thanks!

[Kully]

Instead of adding 4 mocks added 1477ceb, can't we add a simple & single mock to test 4 directions?

Great point, I'll reconfigure this. Thanks for the tip.

Here's what the all-directions mock looks like now
last updated Mar 17, 2021

[archmoj]

After #5653 there is no need for the mock_test.js file.
@Kully would you please merge origin/master into this branch?
Thanks!

[Kully]

@Kully would you please merge origin/master into this branch?

Okay this should be good to go now.

[archmoj]

@Kully now in order for the image test pass please replace test_images/icicle_values_colorscale.png.
See: https://app.circleci.com/pipelines/github/plotly/plotly.js/3891/workflows/8b0c97f3-69ff-4f8c-a264-1e293a46d8f3/jobs/113611/artifacts

The diff is in respect to #5611.

[Kully]

now in order for the image test pass please replace test_images/icicle_values_colorscale.png.

Looks like you already got to it in your commit, thank you 🙂

Is there anything else for us to do now? I can't wait for these ICICLES to hit the main library.

[archmoj]

These repetitive string comparisons could be slow.
Lets add

var isIcicle = type === 'icicle';

to the top and then simply use isIcicle here and below e.g.

x0 += (isIcicle ? pad : pad.l)  - TEXTPAD;

Addressed in c57dda5.

[nicolaskruchten]

this isn't really clear to me... "v" means "bottom up" and "h" means "left to right" ? It would be helpful to note here that the other two possibilities (top-down and right-to-left) can be achieved via flip if that is indeed how to do it.

[nicolaskruchten]

In fact I might phrase this as "the root node(s) are on the left" rather than talking about icicles "growing" :)

[Kully]

Great point, and I agree with this. I'l add that note.

[nicolaskruchten]

I think the default value for this attribute should be 0, so as to look more like sunbursts and less like treemaps

[Kully]

Yes that's a good idea as well. I remember when playing around with padding, 0 or close-to-0 looked the best to me.

Will change this.

[archmoj]

Thanks very much @Kully and @mtwichan for the hard work 🥇
💃 💃