Pie title #2987

codrut3 · 2018-09-08T17:10:34Z

Hi,

I tried to solve #2915 in this PR. I added an attribute title to the pie chart, with the option to show it either inside the hole, or outside.

Showing the title inside is easy. On the other hand, I couldn't come up with an algorithm for placing the title outside that works well in all cases. The problem is that the title may intersect labels / other pies / other images or even go out the chart. So what I do, I consider 2 cases:

there are several pies per column. Then I scale the pie until enough space for the title is left. There is no other solution, because in the following example the pies leave no free space.
if there is one pie per column, I place the title high enough to not intersect pulled slices, and leave space for at most one label below it. It may still intersect slice labels that were moved around, or even be just badly placed. Feel free to suggest another approach :) Maybe one could use an attribute titleheight and let the user decide the optimal placement?

I made some mocks, so you can see how it looks.

The title can be shown either inside the hole (if there is one), or outside. If the title is placed outside, the algorithm tries to position it so that it does not intersect labels or pulled slices.

src/traces/pie/attributes.js

src/traces/pie/defaults.js

src/traces/pie/plot.js

alexcjohnson · 2018-09-11T20:00:52Z

src/traces/pie/plot.js

+    var chartHeight = plotSize.h * (trace.domain.y[1] - trace.domain.y[0]);
+    // leave 3/2 * titlefont.size free space. We need at least titlefont.size
+    // space, and the 1/2 * titlefont.size is a small buffer to avoid the text
+    // touching the pie.


This should be based on titleBB.height - since you can include 'multi<br>line<br>titles'
But a buffer of half the font size is still good.

Actually, in principle anything that depends on the size of text needs to happen in the (potentially async) callback 3rd argument to convertToTspans, in order to handle MathJax (title: '$x+y=z$' etc). That may throw a bit of a wrench into this whole process, since scalePies happens so early on in the process... seems like perhaps all the pie titles need to be rendered upfront, stashed somewhere, and all the existing plotting happens in their combined completion callback. Super annoying, I know...

I handled multiline titles.
Making it work for MathJax is much harder. Maybe I could just use data-notex?

per #2987 (comment) - yes, just use data-notex.

test/jasmine/tests/pie_test.js

test/image/mocks/pie_title_pull.json

test/image/mocks/pie_title_inhole.json

alexcjohnson · 2018-09-11T20:40:20Z

@codrut3 thanks for the PR, this is looking really strong!

One other question that occurred to me is how will these titles interact with outside slice labels? These labels already have known open issues (eg #75) so I won't be surprised if the answer is "not well" and I don't think we need to find a solution to that now, but it's worth keeping in mind and asking whether there's anything simple we could do to improve it in the short term.

codrut3 · 2018-09-13T21:04:50Z

Hi @alexcjohnson @etpinard ,

Thank you for your comments and for taking the time to review my PR! I will probably need a few days to make the changes. I will add all the options that you mentioned to titleposition, pre-render the title and shrink the pie to position it. But I need to think about MathJax. Can't I just set data-notex? 😄

Also, the title may intersect outside labels. I also noticed that outside labels can go outside the plot or overlap other pies. This is, like, a whole issue on its own. I will think about this too.

alexcjohnson · 2018-09-13T22:48:30Z

Can't I just set data-notex?

Actually that's a good point, since we don't support LaTeX in pie slices either for now (see the mathjax test image), I wouldn't be opposed to data-notex here. I suspect there's not too much intersection between pie users and LaTeX users...

This commit changes titleposition to be of the form 'top left', 'top center', ... I added unit tests for correct placement in all possible positions. I also added mocks using multiline titles.

codrut3 · 2018-09-20T20:01:41Z

Hi,
I've made some changes. Can you have a look and tell me if it is fine so far?

I added the following options to titleposition: top left, top center, top right, middle center, bottom left, bottom center, bottom right. I did not add middle left and right, because I am not sure how they would look.

Also, I added unit tests that:

check correct placement of multiline titles in all 7 cases,
check correct placement when some slices are pulled,
check correct scaling of title when placed in the hole ('middle center'),
check correct placing of very large title.

If this is fine, next I will try to do something about titles intersecting outside labels.

Also, note that placing the title to the left (or right) means placing it on the left (or right) margin of the pie (x = center.x - radius). So not the left edge of the subplot, which would be further away. Is this good? What do you think?

alexcjohnson · 2018-09-21T02:45:53Z

test/jasmine/tests/pie_test.js

+            type: 'pie',
+            textinfo: 'none'
+        }], {height: 300, width: 300})
+        .then(_verifyTitle(false, false, true, false, true))


_verifyTitle is beautifully DRY 🌴 ! But you're running it synchronously here, because the argument to then is the result of _verifyTitle, not a function to call when the newPlot promise resolves. Which turns out to be ok, as this plot (with no MathJax or maps) renders synchronously, but it's fragile. My preferred way to handle this is to return a function from _verifyTitle:

function _verifyTitle(checkLeft, checkRight, checkTop, checkBottom, checkMiddleX) { return function() { var title = d3.selectAll('.titletext text'); ... }; }

Done. I've made _verifyTitle return a function.

alexcjohnson · 2018-09-21T02:59:14Z

src/traces/pie/defaults.js

@@ -65,19 +65,17 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
        }
    }

+    handleDomainDefaults(traceOut, layout, coerce);
+
+    var hole = coerce('hole');
    var title = coerce('title');
    if(title) {
        var titlePosition = coerce('titleposition');


I still think we should make the default be 'middle center' when there's a hole (ie var titlePosition = coerce('titleposition', hole ? 'middle center' : 'top center'); and remove attributes.titleposition.dflt) - or maybe when hole > 0.5 or something... would be strange to put the title in the center when the hole is very small!

But I'm not wedded to that idea; if you've considered it and still think 'top center' should be the default in all cases we can keep it that way.

alexcjohnson · 2018-09-21T03:09:36Z

@codrut3 thanks for the changes, they look fantastic! Lovely tests and very thorough test images 🏆

Also, note that placing the title to the left (or right) means placing it on the left (or right) margin of the pie (x = center.x - radius). So not the left edge of the subplot, which would be further away. Is this good? What do you think?

Agreed 💯 - thanks for pointing this out.

If this is fine, next I will try to do something about titles intersecting outside labels.

You're certainly welcome to do that here, but that can also be a separate PR (ideally addressing #75 too).

So the only things I'd still like to see before merging are _verifyTitle #2987 (comment), data-notex #2987 (comment) and finalizing the default titleposition #2987 (comment)

If it is undefined, titleposition is set to 'middle center', if hole > 0, and to 'top center' otherwise. _verifyTitle now returns a promise.

codrut3 · 2018-09-21T19:27:41Z

Hi @alexcjohnson ,
Thank you for the review! I've changed _verifyTitle and titleposition as you say. However, what should I do about data-notex? I set it for the title on line 323. Should I write a test?

etpinard · 2018-09-21T19:28:05Z

Amazing PR @codrut3

Adding this to the 1.42.0 milestone.

alexcjohnson · 2018-09-21T19:54:49Z

Ah sorry, I hadn't noticed that data-notex was already in there, I thought it was still a "should I do this" question. Nothing further is needed. Fantastic work @codrut3, thanks for the quick updates! 💃

@etpinard whenever you're ready to start merging 1.42 features go for it!

etpinard · 2018-09-21T19:56:47Z

whenever you're ready to start merging 1.42 features go for it!

We'll make one more release under 1.41.x next week, then start prepping 1.42.0.

codrut3 · 2018-09-22T17:20:06Z

Hi @alexcjohnson @etpinard ,
Thank you for your nice words! Then I'm looking forward to the release 1.42.0!

I didn't do anything about the title intersecting outside labels. Should I try to do this in another PR?

nicolaskruchten · 2018-09-27T18:04:01Z

Wow, this is really exciting! Hey @ebellumat once this is done I actually would consider adding first-class multi-pie support to PivotTable.js and react-pivottable :)

etpinard · 2018-09-28T16:43:38Z

Starting merging things for 1.42.0 now.

ebellumat · 2018-09-29T15:09:31Z

Wow, this is really exciting! Hey @ebellumat once this is done I actually would consider adding first-class multi-pie support to PivotTable.js and react-pivottable :)

Wow, I was waiting for this, I did not imagine it would be so fast! Congratulations to those involved. And Thanks to @nicolaskruchten to remember that pull request in PivotTable.js

codrut3 added 2 commits September 5, 2018 19:50

Add an optional title to pie charts.

4676fd8

The title can be shown either inside the hole (if there is one), or outside. If the title is placed outside, the algorithm tries to position it so that it does not intersect labels or pulled slices.

Merge branch 'master' into pie_title

79b00ab