Consistent text mode for bar-like & pie-like traces and feature to control text orientation inside pie/sunburst slices by archmoj · Pull Request #4420 · plotly/plotly.js

archmoj · 2019-12-12T03:35:14Z

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 layout attribute called uniformtext with mode (false | 'hide' | 'show'). By enabling this option the text within traces of the same type (namely bar, waterfall, funnel, funnelarea, pie, sunburst and treemap) would be filtered and displayed with an identical text size greater or equal than the value defined by the minsize.

Also this PR adds a new attribute called insidetextorientation to pie and sunburst traces 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

…reemap

etpinard · 2019-12-12T20:52:14Z

 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), {});


This looks like a typo.
varfont=determineFont(tx,d,trace,gd);
should be good enough, right? If not then, we should move the extendFlat call to determineFont.

A copy is needed since the font.size may be replaced by minsize.

Ok that explains the
varfont=Lib.extendFlat({},determineFont(tx,d,trace,gd));
part, but isn't the third argument in
varfont=Lib.extendFlat({},determineFont(tx,d,trace,gd),{});
useless?

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.

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){varout=Lib.extendFlat({},baseFont);out.size=Math.max(baseFont.size,gd._fullLayout.uniformtext.minsize||0);returnout;}
could be a big win!

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.

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.

etpinard · 2019-12-12T20:57:22Z

+ 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); };


What's this new d.pxtxt for?

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.

these are really nice... loving the consistent alignment of labels in the sunburst :)

(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 :)

Good call. Done in 1479943.

etpinard · 2019-12-12T21:12:49Z

@archmoj I still need to try to understand your new insidetextorientation logic and explore all the details you're locking down in the test mocks, but nonetheless here's a first review.

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 calc or crossTraceCalc step - which would unfortunately make them async in general to support MathJax (unless we decide to ditch MathJax for e.g. KaTeX before doing this). Not having to resize the text elements during style would essentially eliminate the transition problems you're referring to in the PR header (example here: https://codepen.io/MojtabaSamimi/pen/NWPrzOG?editors=1000). Personally, these transition problems are minor enough at the moment that I'd vote keeping them as is until we tweak the pipeline.

Before thinking about merging this PR, I'd like to see some jasmine tests to see how things behave when calling ~~relayout~~ (sorry) react with changes in the uniformtext.* attributes.

etpinard · 2019-12-12T21:17:24Z

Moreover, @archmoj could you share a JSON mock or a codepen of a simple case that uses uniformtext? Your test mocks are great for locking multiple things down in one go, but they are a little hard grasp for reviewers.

archmoj · 2019-12-12T22:05:37Z

Moreover, @archmoj could you share a JSON mock or a codepen of a simple case that uses uniformtext? Your test mocks are great for locking multiple things down in one go, but they are a little hard grasp for reviewers.

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

archmoj · 2019-12-19T02:52:42Z

Before thinking about merging this PR, I'd like to see some jasmine tests to see how things behave when calling relayout (sorry) react with changes in the uniformtext.* attributes.

Good call. React tests for uniformtextmode and minsize for bar, watrefall, funnel, funnelarea and pie as well as insidetextorientation for pie are added in 15ce24d.

… transitions - rename pxtxt to textPosAngle and other cleanups - keep textPosAngle inside transform

…tions

…xt orientation

…ition options

- 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

etpinard · 2019-12-23T21:31:20Z

Awesome work @archmoj - this PR looks good to merge 💃

To sum up, this PR:

adds new layout attributes uniformtext.mode and uniformtext.minsize which determines how the font size for various text elements are uniformed between each trace type
add new pie and sunburst attribute insidetextorientation which determines the orientation of text inside slices

archmoj added 5 commits December 11, 2019 20:42

add uniformtext attributes to layout
0d3e289

simplify transform function and expand to reuse in pie and sunburst
c095271

implement uniformtext for bar-like traces as well as funnelarea and t…
b234d7a
…reemap

implement uniformtext and insidetext-orientation for pie and sunburst
f0ac957

add new mocks to test insidetextorientation and uniformtext
4d5fb3d

archmoj added feature something new status: reviewable labels Dec 12, 2019

archmoj added this to the v1.52.0 milestone Dec 12, 2019