Skip to content
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

Fixup texttemplate on log axes #5622

Merged
merged 5 commits into from
May 5, 2021
Merged

Fixup texttemplate on log axes #5622

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d077ee7
refactor function lookup
archmoj May 4, 2021
73d5438
fix issue 5585 for scatter and scattergl
archmoj May 4, 2021
2326b5b
fix issue 5585 for bars
archmoj May 4, 2021
33e1783
add jasmine tests
archmoj May 4, 2021
ca962bc
use c2l instead of c2r
archmoj May 5, 2021
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
3 changes: 2 additions & 1 deletion src/components/drawing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,8 @@ drawing.textPointStyle = function(s, trace, gd) {
}

if(texttemplate) {
var labels = trace._module.formatLabels ? trace._module.formatLabels(d, trace, fullLayout) : {};
var fn = trace._module.formatLabels;
var labels = fn ? fn(d, trace, fullLayout) : {};
var pointValues = {};
appendArrayPointValue(pointValues, trace, d.i);
var meta = trace._meta || {};
Expand Down
4 changes: 4 additions & 0 deletions src/traces/bar/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,10 +647,14 @@ function calcTexttemplate(fullLayout, cd, index, xa, ya) {
}

function formatLabel(u) {
if(pAxis.type === 'log') u = pAxis.c2r(u);

return tickText(pAxis, u, true).text;
}

function formatNumber(v) {
if(vAxis.type === 'log') v = vAxis.c2r(v);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nonblocking, but formatLabel and formatNumber are nearly the same function - just pAxis vs vAxis and the + in the tickText call (any reason that's in one but not the other?) - so should be able to be collapsed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, we may be able to simplify all of this, and get rid of the type === 'log' checks entirely, by using c2l

plotly.js/src/plots/cartesian/set_convert.js

Line 243 in 8cccacc

ax.c2l = (ax.type === 'log') ? toLog : ensureNumber;

And while I'm here, this is ensureNumber:

plotly.js/src/lib/index.js

Lines 166 to 171 in 8cccacc

lib.ensureNumber = function ensureNumber(v) {
if(!isNumeric(v)) return BADNUM;
v = Number(v);
if(v < -FP_SAFE || v > FP_SAFE) return BADNUM;
return isNumeric(v) ? Number(v) : BADNUM;
};

Any reason the last line isn't just return v;? This is a pretty hot function. After already filtering out non-numeric values and converting others to Number explicitly, why would we have to check isNumeric again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we may be able to simplify all of this, and get rid of the type === 'log' checks entirely, by using c2l

plotly.js/src/plots/cartesian/set_convert.js

Line 243 in 8cccacc

ax.c2l = (ax.type === 'log') ? toLog : ensureNumber;

Good call. Applied in ca962bc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ensureNumber comment is tracked in #5634.


return tickText(vAxis, +v, true).text;
}

Expand Down
10 changes: 8 additions & 2 deletions src/traces/scatter/format_labels.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,14 @@ module.exports = function formatLabels(cdi, trace, fullLayout) {
var xa = Axes.getFromTrace(mockGd, trace, 'x');
var ya = Axes.getFromTrace(mockGd, trace, 'y');

labels.xLabel = Axes.tickText(xa, cdi.x, true).text;
labels.yLabel = Axes.tickText(ya, cdi.y, true).text;
var x = cdi.x;
var y = cdi.y;

if(xa.type === 'log') x = xa.c2r(x);
if(ya.type === 'log') y = ya.c2r(y);

labels.xLabel = Axes.tickText(xa, x, true).text;
labels.yLabel = Axes.tickText(ya, y, true).text;

return labels;
};
17 changes: 17 additions & 0 deletions test/jasmine/tests/bar_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2709,6 +2709,23 @@ describe('Text templates on bar traces:', function() {
['%{x}', ['Jan 1, 2019', 'Feb 1, 2019']]
]);

checkTextTemplate({
data: [{
type: 'bar',
textposition: 'outside',
x: [1, 2, 3],
y: [3, 2, 1],
hovertemplate: '%{x}-%{y}',
texttemplate: '%{x}-%{y}'
}],
layout: {
xaxis: {type: 'log'},
yaxis: {type: 'log'},
}
}, 'text.bartext', [
['%{x}-%{y}', ['1-3', '2-2', '3-1']]
]);

checkTextTemplate({
data: [{
type: 'bar',
Expand Down
16 changes: 16 additions & 0 deletions test/jasmine/tests/scatter_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,6 +1241,22 @@ describe('Text templates on scatter traces:', function() {
[['%{y}', '%{x}-%{y}'], ['1', '1-5', '', '']]
]);

checkTextTemplate({
data: [{
type: 'scatter',
mode: 'text',
x: [1, 2, 3],
y: [3, 2, 1],
texttemplate: '%{x}-%{y}'
}],
layout: {
xaxis: {type: 'log'},
yaxis: {type: 'log'},
}
}, '.textpoint', [
['%{x}-%{y}', ['1-3', '2-2', '3-1']]
]);

checkTextTemplate({
data: [{
type: 'scatter',
Expand Down