Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Treemap new trace type #4185
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
Treemap new trace type #4185
Changes from 1 commit
ec6a9de
52ded31
320f81a
06d93ae
6cc8590
6a595b6
671e864
648167d
6dad958
cceee49
f3d38b8
088b0e0
e23a577
eb54f24
0e3185f
fdd69e7
a7b6074
4c85360
65c0e02
ca4b385
0012fa1
544977e
13e0449
57dec38
65f0213
ad24987
36fb510
284bf6d
4e2c328
0cf5bf6
2405ba4
19c7a61
4e3b7f3
e5181ac
e9c0202
e4bc91e
a54ca9c
d7b06a1
c91f9db
ceb13f7
1270d67
35ea4b3
1de80d9
e7e22c6
6130f96
e3a8e91
14674d6
01c5f68
4c7a092
1860826
d2b07b5
1fdf574
3004a9a
4d790a1
1c6e47b
e339bf5
1309bfd
217c0f4
8416883
c918da9
7d67c7a
f8cef49
3c12bf6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse
sunburstAttrs.marker.opacity
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sunburst
does not have amarker.opacity
attribute. But we can reusemarker.line
andmarker.colors
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 6dad958.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the info.
Now I see that
marker.opacity
can multiplybranch.opacity
:plotly.js/src/traces/treemap/style.js
Line 43 in eb54f24
That's interesting, but wouldn't having
marker.opacity
(when set) overridebranch.opacity
be more user friendly in your mind? In particular, if we allowmarker.opacity
to bearrayOk
, this could allow users to set the opacity of a sector pretty easily.What do you think @archmoj ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very good call!
It helped me rethink a couple of things in 0012fa1.
Now the API is more similar to
sunburst
where both have aleaf.opacity
option.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marker.depthfade
?It also occurs to me that opacity might not actually be quite the effect we want - perhaps it would be better to use the opaque color that would result from blending the marker color with the background color in the same proportion? For the bottom-most layer the effect is equivalent, but anything stacked above it currently effectively is more opaque (if the two colors are the same) or blended to a different color (if using a colorscale or explicit colors). Both of these I think are problematic:
Anyway if we extend the analogy I mentioned this morning of "looking at layers of hills in the distance" - those hills are not transparent, they are faded by the air in front of them - which is exactly the same effect as blending with the background color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcjohnson Thanks very much for the feedback. Is there a place in plotly.js code that a similar colour blending is applied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look for color.combine
plotly.js/src/components/color/index.js
Lines 46 to 62 in 56f6983
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New attempt in 4c7a092 - which I'm a fan of 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this solution, especially this part:
plotly.js/src/traces/treemap/style.js
Line 43 in 648167d
which I think gives a nice visual effect while keeping all sectors opaque enough.
As this implementation is a little different than the one proposed in #1038 (comment) and I don't remember us talking about this in a meeting, I'll cc @nicolaskruchten @alexcjohnson @antoinerg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus here is one image test related to that:
https://github.com/plotly/plotly.js/blob/treemap-finalist/test/image/baselines/treemap_pad_mirror.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a small number of levels I think it looks good. For a large number like
treemap_pad_mirror
has, seems like the bottom layer is too transparent. Maybe limit the power to something like 4 max? If that looks weird as you cross the max-height threshold, we could smoothly asymptote with something like:hEffective = (h^(-p) + max^(-p))^(-1/p)
(and insert
hEffective
as the power instead ofpt.height
)p=3, max=4
would be the first one I'd try.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest implementation is in #4185 (comment)