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.

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

merged 5 commits into from
May 5, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented May 4, 2021

Fixes #5585 for scatter and bar.

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels May 4, 2021
@archmoj archmoj added this to the NEXT milestone May 4, 2021
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

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

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.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Consider my comment re: simplifying, but functionally this looks great. Excellent tests.

@archmoj archmoj merged commit 4b73fc3 into master May 5, 2021
@archmoj archmoj deleted the fix5585-log-text branch May 5, 2021 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid text on the logarithmic axis
2 participants