Skip to content

Fix uniformtext and enable coloraxis for sunburst and treemap as well as pathbar.textfont #4444

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 16 commits into from
Jan 6, 2020
Merged

Fix uniformtext and enable coloraxis for sunburst and treemap as well as pathbar.textfont #4444

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
869b97b
Fixes of uniformtext and enable coloraxis for sunburst and treemap
archmoj Dec 25, 2019
b129c5d
no transition with uniformtext for now
archmoj Dec 26, 2019
c630060
only call resizeText not the whole style
archmoj Dec 26, 2019
3737ca9
add jasmine tests for sunburst and treemap transition with uniformtext
archmoj Dec 26, 2019
f6b91e8
tweak new test to pass on CI
archmoj Dec 27, 2019
ef6a952
add comment about special logic to consider values colorscales in sun…
archmoj Dec 27, 2019
2cf7def
make new transition tests more robust
archmoj Dec 29, 2019
6a4b451
treemap pathbar.textfont should have an effect similar to insidetext.…
archmoj Dec 30, 2019
16848b3
improve treemap image test with unifromtext and pathbar custom font
archmoj Dec 30, 2019
79ea1cc
uniform treemap pathbar text
archmoj Dec 31, 2019
2a7721c
textangle bug fix issue 4456
archmoj Dec 31, 2019
ba1ae6f
move all uniformtext functions to bar/uniform_text.js
archmoj Dec 31, 2019
599de22
declare uniform functions on top of bar plot
archmoj Dec 31, 2019
0e3e6e2
simplify (Inside | Outside) transform logic in bar
archmoj Dec 31, 2019
a827bff
Merge branch 'master' into more-unifotrm-text
archmoj Jan 2, 2020
a9c9589
keep text pad consistent when uniformtext rescales at start/end ancho…
archmoj Jan 5, 2020
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
10 changes: 9 additions & 1 deletion src/traces/bar/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ function handleText(traceIn, traceOut, layout, coerce, textposition, opts) {
var moduleHasCliponaxis = !(opts.moduleHasCliponaxis === false);
var moduleHasTextangle = !(opts.moduleHasTextangle === false);
var moduleHasInsideanchor = !(opts.moduleHasInsideanchor === false);
var hasPathbar = !!opts.hasPathbar;

var hasBoth = Array.isArray(textposition) || textposition === 'auto';
var hasInside = hasBoth || textposition === 'inside';
Expand All @@ -147,8 +148,15 @@ function handleText(traceIn, traceOut, layout, coerce, textposition, opts) {
}
coerceFont(coerce, 'insidetextfont', insideTextFontDefault);

if(hasOutside) coerceFont(coerce, 'outsidetextfont', dfltFont);
if(hasPathbar) {
var pathbarTextFontDefault = Lib.extendFlat({}, dfltFont);
if(isColorInheritedFromLayoutFont) {
delete pathbarTextFontDefault.color;
}
coerceFont(coerce, 'pathbar.textfont', pathbarTextFontDefault);
}

if(hasOutside) coerceFont(coerce, 'outsidetextfont', dfltFont);

if(moduleHasSelected) coerce('selected.textfont.color');
if(moduleHasUnselected) coerce('unselected.textfont.color');
Expand Down
30 changes: 23 additions & 7 deletions src/traces/bar/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ function getXY(di, xa, ya, isHorizontal) {
return isHorizontal ? [s, p] : [p, s];
}

function transition(selection, opts, makeOnCompleteCallback) {
if(hasTransition(opts)) {
function transition(selection, fullLayout, opts, makeOnCompleteCallback) {
if(!fullLayout.uniformtext.mode && hasTransition(opts)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? I thought you said this transitions looked fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They did not apply a uniform text size during and after smooth transition.

var onComplete;
if(makeOnCompleteCallback) {
onComplete = makeOnCompleteCallback();
Expand Down Expand Up @@ -91,6 +91,9 @@ function plot(gd, plotinfo, cdModule, traceLayer, opts, makeOnCompleteCallback)
gap: fullLayout.bargap,
groupgap: fullLayout.bargroupgap
};

// don't clear bar when this is called from waterfall or funnel
clearMinTextSize('bar', fullLayout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait. Here you're clearing the min-text-size stash during the plot step. So what's happens when a style-only edit (e.g. Plotly.restyle(gd, 'marker.color', 'red')) gets called?

To me, this logic should probably be somewhere in the calc step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restyle works. Considering interactions (e.g. zoom and selection) this should be cleared after calc and at the stat of plot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering interactions (e.g. zoom and selection) this should be cleared after calc and at the stat of plot.

Can you explain a bit more here? To me, sounds like we should not clear minTextSize during zoom interactions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or let me rephrase my question: if you do move clearMinTextSize to the calc step, do any of the tests fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. In that case the react tests would fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share the branch you used to test that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Summing up a private convo:

  • I was under the impression that the uniformtext attributes were editType: 'calc', so that's why I thought it would be best to clear the min-text-size during the calc step, but they're not. The uniformtext attributes are editType: 'plot'.
  • @archmoj says that making the uniformtext attributes editType: 'calc' would have an impact on the way graph with set uniformtext would behave on zoom
  • Since we'll need to refactor the trace text pipeline at some point (more info in Consistent text mode for bar-like & pie-like traces and feature to control text orientation inside pie/sunburst slices #4420 (comment)), let's keep the clear-min-text logic in the plot step for now.
  • The most important of this PR are the newly added tests.

}

var bartraces = Lib.makeTraceGroups(traceLayer, cdModule, 'trace bars').each(function(cd) {
Expand Down Expand Up @@ -209,13 +212,13 @@ function plot(gd, plotinfo, cdModule, traceLayer, opts, makeOnCompleteCallback)
y1 = fixpx(y1, y0);
}

var sel = transition(Lib.ensureSingle(bar, 'path'), opts, makeOnCompleteCallback);
var sel = transition(Lib.ensureSingle(bar, 'path'), fullLayout, opts, makeOnCompleteCallback);
sel
.style('vector-effect', 'non-scaling-stroke')
.attr('d', 'M' + x0 + ',' + y0 + 'V' + y1 + 'H' + x1 + 'V' + y0 + 'Z')
.call(Drawing.setClipUrl, plotinfo.layerClipId, gd);

if(hasTransition(opts)) {
if(!fullLayout.uniformtext.mode && hasTransition(opts)) {
var styleFns = Drawing.makePointStyleFns(trace);
Drawing.singlePointStyle(di, sel, trace, styleFns, gd);
}
Expand Down Expand Up @@ -409,7 +412,7 @@ function appendBarText(gd, plotinfo, bar, cd, i, x0, x1, y0, y1, opts, makeOnCom
recordMinTextSize(trace.type, transform, fullLayout);
calcBar.transform = transform;

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

Expand All @@ -419,7 +422,7 @@ function recordMinTextSize(
fullLayout // inout
) {
if(fullLayout.uniformtext.mode) {
var minKey = '_' + traceType + 'Text_minsize';
var minKey = getMinKey(traceType);
var minSize = fullLayout.uniformtext.minsize;
var size = transform.scale * transform.fontSize;

Expand All @@ -435,6 +438,18 @@ function recordMinTextSize(
}
}

function clearMinTextSize(
traceType, // in
fullLayout // inout
) {
var minKey = getMinKey(traceType);
fullLayout[minKey] = undefined;
}

function getMinKey(traceType) {
return '_' + traceType + 'Text_minsize';
}

function getRotateFromAngle(angle) {
return (angle === 'auto') ? 0 : angle;
}
Expand Down Expand Up @@ -750,5 +765,6 @@ function calcTextinfo(cd, index, xa, ya) {
module.exports = {
plot: plot,
toMoveInsideBar: toMoveInsideBar,
recordMinTextSize: recordMinTextSize
recordMinTextSize: recordMinTextSize,
clearMinTextSize: clearMinTextSize
};
8 changes: 4 additions & 4 deletions src/traces/bar/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,19 @@ function resizeText(gd, gTrace, traceType) {
if(minSize) {
var shouldHide = fullLayout.uniformtext.mode === 'hide';

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

t.each(function(d) {
gTrace.selectAll(selector).each(function(d) {
var transform = d.transform;
if(transform) {
transform.scale = (shouldHide && transform.hide) ? 0 : minSize / transform.fontSize;
Expand Down
7 changes: 5 additions & 2 deletions src/traces/funnel/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,18 @@
var d3 = require('d3');
var Lib = require('../../lib');
var Drawing = require('../../components/drawing');
var barPlot = require('../bar/plot').plot;
var barPlot = require('../bar/plot');
var clearMinTextSize = barPlot.clearMinTextSize;

module.exports = function plot(gd, plotinfo, cdModule, traceLayer) {
var fullLayout = gd._fullLayout;

clearMinTextSize('funnel', fullLayout);

plotConnectorRegions(gd, plotinfo, cdModule, traceLayer);
plotConnectorLines(gd, plotinfo, cdModule, traceLayer);

barPlot(gd, plotinfo, cdModule, traceLayer, {
barPlot.plot(gd, plotinfo, cdModule, traceLayer, {
mode: fullLayout.funnelmode,
norm: fullLayout.funnelmode,
gap: fullLayout.funnelgap,
Expand Down
3 changes: 3 additions & 0 deletions src/traces/funnelarea/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ var svgTextUtils = require('../../lib/svg_text_utils');
var barPlot = require('../bar/plot');
var toMoveInsideBar = barPlot.toMoveInsideBar;
var recordMinTextSize = barPlot.recordMinTextSize;
var clearMinTextSize = barPlot.clearMinTextSize;

var pieHelpers = require('../pie/helpers');
var piePlot = require('../pie/plot');
Expand All @@ -32,6 +33,8 @@ var formatSliceLabel = piePlot.formatSliceLabel;
module.exports = function plot(gd, cdModule) {
var fullLayout = gd._fullLayout;

clearMinTextSize('funnelarea', fullLayout);

prerenderTitles(cdModule, gd);
layoutAreas(cdModule, fullLayout._size);

Expand Down
6 changes: 5 additions & 1 deletion src/traces/pie/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ var Color = require('../../components/color');
var Drawing = require('../../components/drawing');
var Lib = require('../../lib');
var svgTextUtils = require('../../lib/svg_text_utils');
var recordMinTextSize = require('../bar/plot').recordMinTextSize;
var barPlot = require('../bar/plot');
var recordMinTextSize = barPlot.recordMinTextSize;
var clearMinTextSize = barPlot.clearMinTextSize;

var helpers = require('./helpers');
var eventData = require('./event_data');
Expand All @@ -26,6 +28,8 @@ function plot(gd, cdModule) {
var fullLayout = gd._fullLayout;
var gs = fullLayout._size;

clearMinTextSize('pie', fullLayout);

prerenderTitles(cdModule, gd);
layoutAreas(cdModule, gs);

Expand Down
5 changes: 4 additions & 1 deletion src/traces/sunburst/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(lineWidth) coerce('marker.line.color', layout.paper_bgcolor);

coerce('marker.colors');
var withColorscale = traceOut._hasColorscale = hasColorscale(traceIn, 'marker', 'colors');
var withColorscale = traceOut._hasColorscale = (
hasColorscale(traceIn, 'marker', 'colors') ||
(traceIn.marker || {}).coloraxis // N.B. special logic to consider "values" colorscales
);
if(withColorscale) {
colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: 'marker.', cLetter: 'c'});
}
Expand Down
23 changes: 15 additions & 8 deletions src/traces/sunburst/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,16 @@ function determineOutsideTextFont(trace, pt, layoutFont) {
};
}

function determineInsideTextFont(trace, pt, layoutFont, cont) {
function determineInsideTextFont(trace, pt, layoutFont, opts) {
var onPathbar = (opts || {}).onPathbar;

var cdi = pt.data.data;
var ptNumber = cdi.i;

var customColor = Lib.castOption(trace, ptNumber, 'insidetextfont.color');
var customColor = Lib.castOption(trace, ptNumber,
(onPathbar ? 'pathbar.textfont' : 'insidetextfont') + '.color'
);

if(!customColor && trace._input.textfont) {
// Why not simply using trace.textfont? Because if not set, it
// defaults to layout.font which has a default color. But if
Expand All @@ -98,16 +103,18 @@ function determineInsideTextFont(trace, pt, layoutFont, cont) {

return {
color: customColor || Color.contrast(cdi.color),
family: exports.getInsideTextFontKey('family', cont || trace, pt, layoutFont),
size: exports.getInsideTextFontKey('size', cont || trace, pt, layoutFont)
family: exports.getInsideTextFontKey('family', trace, pt, layoutFont, opts),
size: exports.getInsideTextFontKey('size', trace, pt, layoutFont, opts)
};
}

exports.getInsideTextFontKey = function(keyStr, trace, pt, layoutFont) {
exports.getInsideTextFontKey = function(keyStr, trace, pt, layoutFont, opts) {
var onPathbar = (opts || {}).onPathbar;
var cont = onPathbar ? 'pathbar.textfont' : 'insidetextfont';
var ptNumber = pt.data.data.i;

return (
Lib.castOption(trace, ptNumber, 'insidetextfont.' + keyStr) ||
Lib.castOption(trace, ptNumber, cont + '.' + keyStr) ||
Lib.castOption(trace, ptNumber, 'textfont.' + keyStr) ||
layoutFont.size
);
Expand All @@ -127,10 +134,10 @@ exports.isOutsideText = function(trace, pt) {
return !trace._hasColorscale && exports.isHierarchyRoot(pt);
};

exports.determineTextFont = function(trace, pt, layoutFont, cont) {
exports.determineTextFont = function(trace, pt, layoutFont, opts) {
return exports.isOutsideText(trace, pt) ?
determineOutsideTextFont(trace, pt, layoutFont) :
determineInsideTextFont(trace, pt, layoutFont, cont);
determineInsideTextFont(trace, pt, layoutFont, opts);
};

exports.hasTransition = function(transitionOpts) {
Expand Down
16 changes: 12 additions & 4 deletions src/traces/sunburst/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ var d3Hierarchy = require('d3-hierarchy');
var Drawing = require('../../components/drawing');
var Lib = require('../../lib');
var svgTextUtils = require('../../lib/svg_text_utils');
var recordMinTextSize = require('../bar/plot').recordMinTextSize;
var barPlot = require('../bar/plot');
var recordMinTextSize = barPlot.recordMinTextSize;
var clearMinTextSize = barPlot.clearMinTextSize;
var piePlot = require('../pie/plot');
var computeTransform = piePlot.computeTransform;
var transformInsideText = piePlot.transformInsideText;
var styleOne = require('./style').styleOne;

var resizeText = require('../bar/style').resizeText;
var attachFxHandlers = require('./fx');
var constants = require('./constants');
var helpers = require('./helpers');
Expand All @@ -32,7 +34,9 @@ exports.plot = function(gd, cdmodule, transitionOpts, makeOnCompleteCallback) {
// If transition config is provided, then it is only a partial replot and traces not
// updated are removed.
var isFullReplot = !transitionOpts;
var hasTransition = helpers.hasTransition(transitionOpts);
var hasTransition = !fullLayout.uniformtext.mode && helpers.hasTransition(transitionOpts);

clearMinTextSize('sunburst', fullLayout);

join = layer.selectAll('g.trace.sunburst')
.data(cdmodule, function(cd) { return cd[0].trace.uid; });
Expand Down Expand Up @@ -70,6 +74,10 @@ exports.plot = function(gd, cdmodule, transitionOpts, makeOnCompleteCallback) {
join.each(function(cd) {
plotOne(gd, cd, this, transitionOpts);
});

if(fullLayout.uniformtext.mode) {
resizeText(gd, fullLayout._sunburstlayer.selectAll('.trace'), 'sunburst');
}
}

if(isFullReplot) {
Expand All @@ -79,7 +87,7 @@ exports.plot = function(gd, cdmodule, transitionOpts, makeOnCompleteCallback) {

function plotOne(gd, cd, element, transitionOpts) {
var fullLayout = gd._fullLayout;
var hasTransition = helpers.hasTransition(transitionOpts);
var hasTransition = !fullLayout.uniformtext.mode && helpers.hasTransition(transitionOpts);

var gTrace = d3.select(element);
var slices = gTrace.selectAll('g.slice');
Expand Down
11 changes: 7 additions & 4 deletions src/traces/treemap/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,11 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('hovertext');
coerce('hovertemplate');

var hasPathbar = coerce('pathbar.visible');

var textposition = 'auto';
handleText(traceIn, traceOut, layout, coerce, textposition, {
hasPathbar: hasPathbar,
moduleHasSelected: false,
moduleHasUnselected: false,
moduleHasConstrain: false,
Expand All @@ -73,7 +76,10 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(lineWidth) coerce('marker.line.color', layout.paper_bgcolor);

var colors = coerce('marker.colors');
var withColorscale = traceOut._hasColorscale = hasColorscale(traceIn, 'marker', 'colors');
var withColorscale = traceOut._hasColorscale = (
hasColorscale(traceIn, 'marker', 'colors') ||
(traceIn.marker || {}).coloraxis // N.B. special logic to consider "values" colorscales
);
if(withColorscale) {
colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: 'marker.', cLetter: 'c'});
} else {
Expand All @@ -100,10 +106,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}
};

var hasPathbar = coerce('pathbar.visible');
if(hasPathbar) {
Lib.coerceFont(coerce, 'pathbar.textfont', layout.font);

// This works even for multi-line labels as treemap pathbar trim out line breaks
coerce('pathbar.thickness', traceOut.pathbar.textfont.size + 2 * TEXTPAD);

Expand Down
19 changes: 5 additions & 14 deletions src/traces/treemap/draw_ancestors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) {
}

updateSlices.each(function(pt) {
pt._hoverX = viewX(pt.x1 - height / 2);
pt._hoverX = viewX(pt.x1 - Math.min(width, height) / 2);
pt._hoverY = viewY(pt.y1 - height / 2);

var sliceTop = d3.select(this);
Expand Down Expand Up @@ -141,7 +141,9 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) {
s.attr('data-notex', 1);
});

var font = Lib.ensureUniformFontSize(gd, helpers.determineTextFont(trace, pt, fullLayout.font, trace.pathdir));
var font = Lib.ensureUniformFontSize(gd, helpers.determineTextFont(trace, pt, fullLayout.font, {
onPathbar: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this commit made three existing mocks change (treemap_level-depth, treemap_packings and treemap_textposition) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question.
And a bad mistake here.
There is no such a thing as pathdir!
It should have been written pathbar.

}));

sliceText.text(pt._text || ' ') // use one space character instead of a blank string to avoid jumps during transition
.classed('slicetext', true)
Expand All @@ -151,22 +153,11 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) {

pt.textBB = Drawing.bBox(sliceText.node());
pt.transform = toMoveInsideSlice(pt, {
fontSize: font.size,
onPathbar: true
});

pt.transform.fontSize = font.size;

if(helpers.isOutsideText(trace, pt)) {
// consider in/out diff font sizes
var outsideFont = helpers.getOutsideTextFontKey('size', trace, pt, fullLayout.font);
var insideFont = helpers.getInsideTextFontKey('size', trace, pt, fullLayout.font);

var diffFontSize = outsideFont - insideFont;

pt.transform.targetY -= diffFontSize;
pt.transform.fontSize -= diffFontSize;
}

if(hasTransition) {
sliceText.transition().attrTween('transform', function(pt2) {
var interp = makeUpdateTextInterpolator(pt2, onPathbar, refRect, [width, height]);
Expand Down
1 change: 1 addition & 0 deletions src/traces/treemap/draw_descendants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) {

pt.textBB = Drawing.bBox(sliceText.node());
pt.transform = toMoveInsideSlice(pt, {
fontSize: font.size,
isHeader: isHeader
});
pt.transform.fontSize = font.size;
Expand Down
Loading