Consistent text mode for bar-like & pie-like traces and feature to control text orientation inside pie/sunburst slices #4420
Conversation
src/traces/bar/style.js
Outdated
| @@ -57,7 +95,9 @@ function stylePoints(sel, trace, gd) { | |||
| function styleTextPoints(sel, trace, gd) { | |||
| sel.selectAll('text').each(function(d) { | |||
| var tx = d3.select(this); | |||
| var font = determineFont(tx, d, trace, gd); | |||
| var font = Lib.extendFlat({}, determineFont(tx, d, trace, gd), {}); | |||
There was a problem hiding this comment.
This looks like a typo.
var font = determineFont(tx, d, trace, gd);should be good enough, right? If not then, we should move the extendFlat call to determineFont.
There was a problem hiding this comment.
A copy is needed since the font.size may be replaced by minsize.
There was a problem hiding this comment.
Ok that explains the
var font = Lib.extendFlat({}, determineFont(tx, d, trace, gd));part, but isn't the third argument in
var font = Lib.extendFlat({}, determineFont(tx, d, trace, gd), {});useless?
There was a problem hiding this comment.
Correct. We could/should drop that.
I can make a centralize function for those font getters.
Should uniform text.minsize override the font.size(s) defined by traces? If not we could use the minimum among them.
There was a problem hiding this comment.
Should uniform text.minsize override the font.size(s) defined by traces?
Like override them in gd.data[i] and gd._fullData[i]? If so, then the answer is NO. We shouldn't mutate that.
If not we could use the minimum among them.
That sounds about right. Adding something like
Lib.applyUniformtext = function(gd, baseFont) {
var out = Lib.extendFlat({}, baseFont);
out.size = Math.max(baseFont.size, gd._fullLayout.uniformtext.minsize || 0);
return out;
}could be a big win!
There was a problem hiding this comment.
Thanks for the hint.
Would you please clarify which size should be used in the scenario below?
When one of the traces defined its font size to be e.g. 8; while the uniform.minsize is set to 16.
In the current implementation 16 wins over 8.
There was a problem hiding this comment.
When one of the traces defined its font size to be e.g. 8; while the uniform.minsize is set to 16.
That's a hard one. I can see arguments for both ways. On one hard, it would make things more flexible if we allow users to override the uniform-text-size results. But then again, if they really want that, they can set all the textfont.size in the traces manually w/o even touching uniformtext.
src/traces/sunburst/plot.js
Outdated
| var getTargetX = function(d) { return cx + (d.pxtxt || d.pxmid)[0] * d.transform.rCenter + (d.transform.x || 0); }; | ||
| var getTargetY = function(d) { return cy + (d.pxtxt || d.pxmid)[1] * d.transform.rCenter + (d.transform.y || 0); }; |
There was a problem hiding this comment.
What's this new d.pxtxt for?
There was a problem hiding this comment.
When fitting horizontal text in pie and sunburst the best position may not be the middle point of the arc. So pxtxt keeps the position of text if it is moved form the middle.
There was a problem hiding this comment.
these are really nice... loving the consistent alignment of labels in the sunburst :)
There was a problem hiding this comment.
(well, semi-consistent!) We should maybe special-case the noon and 6 o'clock positions as 'preferred' if they result in the same size as 12:05 or 6:15, say :)
There was a problem hiding this comment.
Good call. Done in 1479943.
|
@archmoj I still need to try to understand your new Writing what we discussed a week ago: in a not-so-distant future (i.e. whenever we'll attempt to implement #2000) we should try to compute the bounding boxes of all the trace text items before plotting them. In brief, this means adding logic to the Before thinking about merging this PR, I'd like to see some jasmine tests to see how things behave when calling |
|
Moreover, @archmoj could you share a JSON mock or a codepen of a simple case that uses |
Here is a simple demo which also added to the PR description. |
- make pie insidetextorientation coerce condition more readable - no need for extra check before recompute bounding box - revise uniformtext attribute descriptions - remove boolean from getTextTransform arguments - add js doc description - create and use Lib.ensureUniformFontSize - make the hide/show logic independent of _module.plot - refactor textposition coerce logic - add test for inside-text-orientation coerce logic with various textposition scenarios - use radial tangential and horizontal in inside-text-orientation keys
- bypass points without text - add react tests for bar, waterfall, funnel, funnelarea, pie uniform-text options - add react tests for pie inside-text-orientation options
Good call. React tests for |
… transitions - rename pxtxt to textPosAngle and other cleanups - keep textPosAngle inside transform
- use 14 (instead of 16) as "greater than fontsize dflt" `uniformtext.size` value - add tolenrence to selected assertion in sunburst and treemap tween text assertions
- also fixed parsing rotation and scale values from transform
|
Awesome work @archmoj - this PR looks good to merge 💃 To sum up, this PR:
|
Resolves #4247 and
resolves #3590
In addition to simplifying (e.g. removing extra translations) and unifying transformations between bar-like and pie-like traces, this PR adds a new
layoutattribute calleduniformtextwithmode(false | 'hide' | 'show'). By enabling this option the text within traces of the same type (namelybar,waterfall,funnel,funnelarea,pie,sunburstandtreemap) would be filtered and displayed with an identical text size greater or equal than the value defined by theminsize.Also this PR adds a new attribute called
insidetextorientationtopieandsunbursttraces which allows more control over the orientation of text within the slices.Keeping text consistent during transitions is not within the scope of this PR; but it may be addressed by a separated PR.
simple demo
sunburst text orientation demo
@plotly/plotly_js
cc: @nicolaskruchten