Skip to content

Consistent text mode for bar-like & pie-like traces and feature to control text orientation inside pie/sunburst slices #4420

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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Dec 23, 2019
Merged

Consistent text mode for bar-like & pie-like traces and feature to control text orientation inside pie/sunburst slices #4420

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0d3e289
add uniformtext attributes to layout
archmoj Dec 12, 2019
c095271
simplify transform function and expand to reuse in pie and sunburst
archmoj Dec 12, 2019
b234d7a
implement uniformtext for bar-like traces as well as funnelarea and t…
archmoj Dec 12, 2019
f0ac957
implement uniformtext and insidetext-orientation for pie and sunburst
archmoj Dec 12, 2019
4d5fb3d
add new mocks to test insidetextorientation and uniformtext
archmoj Dec 12, 2019
a27dc29
Consider first review
archmoj Dec 13, 2019
1479943
make tangential go towards noon and 6 and make radial go towards 3 and 9
archmoj Dec 16, 2019
eab205a
rewrite uniform text style
archmoj Dec 16, 2019
5348c58
add new mocks cases by Nicolas
archmoj Dec 18, 2019
15ce24d
apply zero scale for hiding text elements
archmoj Dec 19, 2019
0840d82
add new mocks using both inside-text-orientation and uniformtext
archmoj Dec 19, 2019
328ffd0
move reposition logic outside pie transformInsideText function
archmoj Dec 19, 2019
2a31685
apply next text position when it is not at the center
archmoj Dec 19, 2019
d1b50b1
Apply polar coordinates to move text inside the slice during sunburst…
archmoj Dec 19, 2019
2cb5687
sunburst handle no text when using various inside-text-orientation op…
archmoj Dec 20, 2019
beed042
add react tests for treemap and pie uniform text as well as inside te…
archmoj Dec 20, 2019
51d7e1b
add tests for updating text positions using different inside-text-pos…
archmoj Dec 20, 2019
603e0cc
wip
etpinard Dec 20, 2019
8784274
first attempt at fixing tests on etpinard's laptop
etpinard Dec 23, 2019
2686c46
Fix funnelarea, pie, sunburst and treemap tests
archmoj Dec 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 65 additions & 30 deletions src/traces/bar/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,14 +237,14 @@ function plot(gd, plotinfo, cdModule, traceLayer, opts, makeOnCompleteCallback)
Registry.getComponentMethod('errorbars', 'plot')(gd, bartraces, plotinfo, opts);
}

function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, makeOnCompleteCallback) {
function appendBarText(gd, plotinfo, bar, cd, i, x0, x1, y0, y1, opts, makeOnCompleteCallback) {
var xa = plotinfo.xaxis;
var ya = plotinfo.yaxis;

var fullLayout = gd._fullLayout;
var textPosition;

function appendTextNode(bar, text, textFont) {
function appendTextNode(bar, text, font) {
var textSelection = Lib.ensureSingle(bar, 'text')
.text(text)
.attr({
Expand All @@ -254,25 +254,25 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, ma
// tex and regular text together
'data-notex': 1
})
.call(Drawing.font, textFont)
.call(Drawing.font, font)
.call(svgTextUtils.convertToTspans, gd);

return textSelection;
}

// get trace attributes
var trace = calcTrace[0].trace;
var trace = cd[0].trace;
var isHorizontal = (trace.orientation === 'h');

var text = getText(fullLayout, calcTrace, i, xa, ya);
var text = getText(fullLayout, cd, i, xa, ya);
textPosition = getTextPosition(trace, i);

// compute text position
var inStackOrRelativeMode =
opts.mode === 'stack' ||
opts.mode === 'relative';

var calcBar = calcTrace[i];
var calcBar = cd[i];
var isOutmostBar = !inStackOrRelativeMode || calcBar._outmost;

if(!text ||
Expand All @@ -285,7 +285,7 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, ma
}

var layoutFont = fullLayout.font;
var barColor = style.getBarColor(calcTrace[i], trace);
var barColor = style.getBarColor(cd[i], trace);
var insideTextFont = style.getInsideTextFont(trace, i, layoutFont, barColor);
var outsideTextFont = style.getOutsideTextFont(trace, i, layoutFont);

Expand Down Expand Up @@ -318,6 +318,7 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, ma
var textBB;
var textWidth;
var textHeight;
var font;

if(textPosition === 'outside') {
if(!isOutmostBar && !calcBar.hasB) textPosition = 'inside';
Expand All @@ -327,7 +328,11 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, ma
if(isOutmostBar) {
// draw text using insideTextFont and check if it fits inside bar
textPosition = 'inside';
textSelection = appendTextNode(bar, text, insideTextFont);

font = Lib.extendFlat({}, insideTextFont, {});
font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);

textSelection = appendTextNode(bar, text, font);

textBB = Drawing.bBox(textSelection.node()),
textWidth = textBB.width,
Expand Down Expand Up @@ -357,9 +362,10 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, ma
}

if(!textSelection) {
textSelection = appendTextNode(bar, text,
(textPosition === 'outside') ?
outsideTextFont : insideTextFont);
font = Lib.extendFlat({}, (textPosition === 'outside') ? outsideTextFont : insideTextFont, {});
font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);

textSelection = appendTextNode(bar, text, font);

var currentTransform = textSelection.attr('transform');
textSelection.attr('transform', '');
Expand All @@ -374,32 +380,61 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, ma
}
}

var angle = trace.textangle;

// compute text transform
var transform, constrained;
if(textPosition === 'outside') {
constrained =
trace.constraintext === 'both' ||
trace.constraintext === 'outside';

transform = Lib.getTextTransform(toMoveOutsideBar(x0, x1, y0, y1, textBB, {
transform = toMoveOutsideBar(x0, x1, y0, y1, textBB, {
isHorizontal: isHorizontal,
constrained: constrained,
angle: trace.textangle
}));
angle: angle
});
} else {
constrained =
trace.constraintext === 'both' ||
trace.constraintext === 'inside';

transform = Lib.getTextTransform(toMoveInsideBar(x0, x1, y0, y1, textBB, {
transform = toMoveInsideBar(x0, x1, y0, y1, textBB, {
isHorizontal: isHorizontal,
constrained: constrained,
angle: trace.textangle,
angle: angle,
anchor: trace.insidetextanchor
}));
});
}

transition(textSelection, opts, makeOnCompleteCallback).attr('transform', transform);
transform.fontSize = font.size;
recordMinTextSize(trace.type, transform, fullLayout);
calcBar.transform = transform;

transition(textSelection, opts, makeOnCompleteCallback)
.attr('transform', Lib.getTextTransform(transform));
}

function recordMinTextSize(
traceType, // in
transform, // inout
fullLayout // inout
) {
if(fullLayout.uniformtext.mode) {
var minKey = '_' + traceType + 'Text_minsize';
var minSize = fullLayout.uniformtext.minsize;
var size = transform.scale * transform.fontSize;

transform.hide = size < minSize;

fullLayout[minKey] = fullLayout[minKey] || Infinity;
if(!transform.hide) {
fullLayout[minKey] = Math.min(
fullLayout[minKey],
Math.max(size, minSize)
);
}
}
}

function getRotateFromAngle(angle) {
Expand Down Expand Up @@ -549,15 +584,15 @@ function toMoveOutsideBar(x0, x1, y0, y1, textBB, opts) {
};
}

function getText(fullLayout, calcTrace, index, xa, ya) {
var trace = calcTrace[0].trace;
function getText(fullLayout, cd, index, xa, ya) {
var trace = cd[0].trace;
var texttemplate = trace.texttemplate;

var value;
if(texttemplate) {
value = calcTexttemplate(fullLayout, calcTrace, index, xa, ya);
value = calcTexttemplate(fullLayout, cd, index, xa, ya);
} else if(trace.textinfo) {
value = calcTextinfo(calcTrace, index, xa, ya);
value = calcTextinfo(cd, index, xa, ya);
} else {
value = helpers.getValue(trace.text, index);
}
Expand All @@ -570,8 +605,8 @@ function getTextPosition(trace, index) {
return helpers.coerceEnumerated(attributeTextPosition, value);
}

function calcTexttemplate(fullLayout, calcTrace, index, xa, ya) {
var trace = calcTrace[0].trace;
function calcTexttemplate(fullLayout, cd, index, xa, ya) {
var trace = cd[0].trace;
var texttemplate = Lib.castOption(trace, index, 'texttemplate');
if(!texttemplate) return '';
var isWaterfall = (trace.type === 'waterfall');
Expand Down Expand Up @@ -599,7 +634,7 @@ function calcTexttemplate(fullLayout, calcTrace, index, xa, ya) {
return tickText(vAxis, +v, true).text;
}

var cdi = calcTrace[index];
var cdi = cd[index];
var obj = {};

obj.label = cdi.p;
Expand Down Expand Up @@ -640,8 +675,8 @@ function calcTexttemplate(fullLayout, calcTrace, index, xa, ya) {
return Lib.texttemplateString(texttemplate, obj, fullLayout._d3locale, pt, obj, trace._meta || {});
}

function calcTextinfo(calcTrace, index, xa, ya) {
var trace = calcTrace[0].trace;
function calcTextinfo(cd, index, xa, ya) {
var trace = cd[0].trace;
var isHorizontal = (trace.orientation === 'h');
var isWaterfall = (trace.type === 'waterfall');
var isFunnel = (trace.type === 'funnel');
Expand All @@ -657,7 +692,7 @@ function calcTextinfo(calcTrace, index, xa, ya) {
}

var textinfo = trace.textinfo;
var cdi = calcTrace[index];
var cdi = cd[index];

var parts = textinfo.split('+');
var text = [];
Expand All @@ -666,7 +701,7 @@ function calcTextinfo(calcTrace, index, xa, ya) {
var hasFlag = function(flag) { return parts.indexOf(flag) !== -1; };

if(hasFlag('label')) {
text.push(formatLabel(calcTrace[index].p));
text.push(formatLabel(cd[index].p));
}

if(hasFlag('text')) {
Expand Down Expand Up @@ -717,5 +752,5 @@ function calcTextinfo(calcTrace, index, xa, ya) {
module.exports = {
plot: plot,
toMoveInsideBar: toMoveInsideBar,
toMoveOutsideBar: toMoveOutsideBar
recordMinTextSize: recordMinTextSize
};
46 changes: 44 additions & 2 deletions src/traces/bar/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,46 @@ var attributeInsideTextFont = attributes.insidetextfont;
var attributeOutsideTextFont = attributes.outsidetextfont;
var helpers = require('./helpers');

function resizeText(gd, gTrace, traceType) {
var fullLayout = gd._fullLayout;
var minSize = fullLayout['_' + traceType + 'Text_minsize'];
if(minSize) {
var shouldHide = fullLayout.uniformtext.mode === 'hide';

var t;
switch(traceType) {
case 'funnelarea' :
case 'pie' :
case 'sunburst' :
case 'treemap' :
t = gTrace.selectAll('g.slicetext').selectAll('text');
break;
default :
t = gTrace.selectAll('g.points').selectAll('g.point').selectAll('text');
}

var isCenter = (
traceType === 'pie' ||
traceType === 'sunburst'
);

t.each(function(d) {
var transform = d.transform;

transform.scale = minSize / transform.fontSize;
d3.select(this).attr('transform', Lib.getTextTransform(transform, isCenter));

if(shouldHide && transform.hide) {
d3.select(this).text(' ');
}
});
}
}

function style(gd) {
var s = d3.select(gd).selectAll('g.barlayer').selectAll('g.trace');
resizeText(gd, s, 'bar');

var barcount = s.size();
var fullLayout = gd._fullLayout;

Expand Down Expand Up @@ -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), {});
Copy link
Contributor

@etpinard etpinard Dec 12, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@archmoj archmoj Dec 12, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@etpinard etpinard Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@archmoj archmoj Dec 12, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@etpinard etpinard Dec 13, 2019

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@archmoj archmoj Dec 13, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@etpinard etpinard Dec 13, 2019

Choose a reason for hiding this comment

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

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.

font.size = Math.max(font.size, gd._fullLayout.uniformtext.minsize || 0);

Drawing.font(tx, font);
});
}
Expand Down Expand Up @@ -85,6 +125,7 @@ function styleTextInSelectionMode(txs, trace, gd) {

if(d.selected) {
font = Lib.extendFlat({}, determineFont(tx, d, trace, gd));
font.size = Math.max(font.size, gd._fullLayout.uniformtext.minsize || 0);

var selectedFontColor = trace.selected.textfont && trace.selected.textfont.color;
if(selectedFontColor) {
Expand Down Expand Up @@ -171,5 +212,6 @@ module.exports = {
styleOnSelect: styleOnSelect,
getInsideTextFont: getInsideTextFont,
getOutsideTextFont: getOutsideTextFont,
getBarColor: getBarColor
getBarColor: getBarColor,
resizeText: resizeText
};
6 changes: 4 additions & 2 deletions src/traces/funnel/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ var d3 = require('d3');
var Drawing = require('../../components/drawing');
var Color = require('../../components/color');
var DESELECTDIM = require('../../constants/interactions').DESELECTDIM;

var styleTextPoints = require('../bar/style').styleTextPoints;
var barStyle = require('../bar/style');
var resizeText = barStyle.resizeText;
var styleTextPoints = barStyle.styleTextPoints;

function style(gd, cd, sel) {
var s = sel ? sel : d3.select(gd).selectAll('g.funnellayer').selectAll('g.trace');
resizeText(gd, s, 'funnel');

s.style('opacity', function(d) { return d[0].trace.opacity; });

Expand Down
Loading