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.
Pie title #2987
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
Changes from 2 commits
4676fd8
79b00ab
ed86959
96e3afd
97cc9bb
bce5add
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.
I still think we should make the default be
'middle center'
when there's ahole
(ievar titlePosition = coerce('titleposition', hole ? 'middle center' : 'top center');
and removeattributes.titleposition.dflt
) - or maybe whenhole > 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.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.
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.
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, sincescalePies
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...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 handled multiline titles.
Making it work for MathJax is much harder. Maybe I could just use
data-notex
?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.
per #2987 (comment) - yes, just use
data-notex
.