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

Pie title #2987

Merged
merged 6 commits into from
Sep 28, 2018
Merged

Pie title #2987

merged 6 commits into from
Sep 28, 2018

Conversation

codrut3
Copy link
Contributor

@codrut3 codrut3 commented Sep 8, 2018

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.
    newplot

  • 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.
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@alexcjohnson
Copy link
Collaborator

@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
Copy link
Contributor Author

codrut3 commented Sep 13, 2018

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
Copy link
Collaborator

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... :neckbeard:

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
Copy link
Contributor Author

codrut3 commented Sep 20, 2018

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?

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

@alexcjohnson alexcjohnson Sep 21, 2018

Choose a reason for hiding this comment

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

_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');
        ...
    };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've made _verifyTitle return a function.

@@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@alexcjohnson
Copy link
Collaborator

@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
Copy link
Contributor Author

codrut3 commented Sep 21, 2018

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
Copy link
Contributor

Amazing PR @codrut3

Adding this to the 1.42.0 milestone.

@etpinard etpinard added this to the v1.42.0 milestone Sep 21, 2018
@alexcjohnson
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Contributor Author

codrut3 commented Sep 22, 2018

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
Copy link
Contributor

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
Copy link
Contributor

Starting merging things for 1.42.0 now.

@etpinard etpinard merged commit c2f868c into plotly:master Sep 28, 2018
@ebellumat
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants