Skip to content

Commit

Permalink
pie & sunburst: pass empty text string to texttemplate
Browse files Browse the repository at this point in the history
  • Loading branch information
antoinerg committed Sep 10, 2019
1 parent 9b4a38d commit 2e966e0
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
4 changes: 2 additions & 2 deletions src/traces/pie/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -991,8 +991,8 @@ function formatSliceLabel(gd, pt, cd0) {
pt.text = '';
} else {
var obj = makeTemplateVariables(pt);
var ptTx = helpers.getFirstFilled(trace.text, pt.pts);
if(isValidTextValue(ptTx)) obj.text = ptTx;
var ptTx = Lib.castOption(trace, pt.i, 'text');

This comment has been minimized.

Copy link
@etpinard

etpinard Sep 10, 2019

Contributor

Hmm. I think we need to keep helpers.getFirstFilled here to handle cases with aggregated labels e.g.

https://rreusser.github.io/plotly-mock-viewer/#pie_aggregated

By the looks like of it, we don't have any tests for hover on aggregated pies.

This comment has been minimized.

Copy link
@antoinerg

antoinerg Sep 10, 2019

Author Contributor

Fixed in cb40618

This comment has been minimized.

Copy link
@etpinard

etpinard Sep 10, 2019

Contributor

Thanks! Would we mind adding a test for aggregated pie + texttemplate

if(isValidTextValue(ptTx) || ptTx === '') obj.text = ptTx;
pt.text = Lib.texttemplateString(txt, obj, gd._fullLayout._d3locale, obj, trace._meta || {});
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/traces/sunburst/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ function formatSliceLabel(pt, trace, fullLayout) {
obj.color = cdi.color;
}
var ptTx = Lib.castOption(trace, cdi.i, 'text');
if(Lib.isValidTextValue(ptTx)) obj.text = ptTx;
if(Lib.isValidTextValue(ptTx) || ptTx === '') obj.text = ptTx;

This comment has been minimized.

Copy link
@archmoj

archmoj Sep 10, 2019

Contributor

Non-blocking: slight performance gain if we test for the blank first.

This comment has been minimized.

Copy link
@antoinerg

antoinerg Sep 10, 2019

Author Contributor

I wonder if we would gain much in real-world usage considering we will rarely get empty strings but thanks for pointing this out :)

obj.customdata = Lib.castOption(trace, cdi.i, 'customdata');
return Lib.texttemplateString(txt, obj, fullLayout._d3locale, obj, trace._meta || {});
}
Expand Down

0 comments on commit 2e966e0

Please sign in to comment.