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

Add marker.cornerradius to treemap #6351

Merged
merged 11 commits into from Dec 21, 2022
Merged

Add marker.cornerradius to treemap #6351

merged 11 commits into from Dec 21, 2022

Conversation

emilykl
Copy link
Contributor

@emilykl emilykl commented Oct 24, 2022

This PR exposes the marker.fillet attribute for treemap and icicle plots, which allows users to create plots with rounded corners by setting the fillet radius in pixels.

Based on this comment from @archmoj.

Fillet radius is limited to half of the smallest dimension of a rectangle to avoid weird artifacts on small rectangles.

Example with marker.fillet set to 4:
treemap_packages_colorscale_novalue-2

@nicolaskruchten
Copy link
Member

wow that looks great! Any chance we could get this on bars? :) #2196 is a long-standing one... Just need to make sure it's only the top-most bar in a stack.

@archmoj
Copy link
Contributor

archmoj commented Oct 24, 2022

wow that looks great! Any chance we could get this on bars? :) #2196 is a long-standing one... Just need to make sure it's only the top-most bar in a stack.

Yes we are paving the way for bars here. But that should be a separate PR.
@nicolaskruchten your suggestions in terms of attribute naming and API design as always would be much appreciated.

@nicolaskruchten
Copy link
Member

Hehe ok. I would vote for something like marker.cornerradius or something?

fillet: {
valType: 'number',
min: 0,
// max: 10, // TODO: Do we need a max?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's possible to make the radius so big that the text or smaller inner boxes poke out of their containers. Here's your mock bumped up to radius 40:
Screen Shot 2022-10-27 at 19 19 21
How about we make a dynamic max? If I did the math right, a safe value would be x+y+sqrt(2*x*y) where x is min(padding.l, padding.r) and y is min(padding.t, padding.b)
Actually might be more user-friendly to leave max undefined, but restrict it during rendering and describe this in the attribute description - since we're restricting the radius anyway if the box gets too small... something like "the actual radius we draw will be limited to half the box size, or smaller if necessary to prevent the box contents from overflowing its edges"

@alexcjohnson
Copy link
Contributor

I like marker.cornerradius - a little verbose but totally unambiguous.

Looks beautiful for treemaps (if you don't try to break it like I did 😈 )

For icicle though, can we NOT round the edges where the parts abut the whole? Kind of the analog of bars "only the top bar" though I think rounding both sides is fine. I just don't like it rounded in the middle, as I think it obscures the "part of a whole" model. Also looks particularly bad where a very small slice isn't touching its parent at all:
Screen Shot 2022-10-27 at 19 03 20
If that's going to be too annoying to implement, perhaps better to leave icicle out of this pr?

@archmoj archmoj changed the title Expose marker.fillet attribute for rounded corners in treemap and icicle plots Add marker.cornerradius to treemap Nov 17, 2022
@alexcjohnson
Copy link
Contributor

@archmoj is my max radius comment #6351 (comment) still open?

@archmoj
Copy link
Contributor

archmoj commented Nov 21, 2022

@archmoj is my max radius comment #6351 (comment) still open?

Now addressed by a814408.

@alexcjohnson
Copy link
Contributor

It's still able to generate too big of a radius. On the latest commit if I load treemap_packages_colorscale_novalue and then run Plotly.restyle(gd,'marker.cornerradius', 40), the result is the same image as in #6351 (comment)

@archmoj
Copy link
Contributor

archmoj commented Dec 14, 2022

Plotly.restyle(gd,'marker.cornerradius', 40)

It don't get the similar image. Perhaps you should git pull recent changes in your local branch?

Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, I had the latest commit but somehow was still getting the old behavior. Yes, it looks good now! 💃

@archmoj archmoj merged commit 984a691 into master Dec 21, 2022
@archmoj archmoj deleted the treemap-round-corners branch December 21, 2022 14:31
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.

None yet

4 participants