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

Format numbers in waterfall hover #3711

Merged
merged 2 commits into from Apr 2, 2019

Conversation

Projects
None yet
3 participants
@archmoj
Copy link
Collaborator

commented Apr 2, 2019

Fix #3710 by rounding 10 digits then parsing as float.
Codepen.
@plotly/plotly_js

@@ -34,16 +38,16 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
if(!di.isSum) {
// format delta numbers:
if(size > 0) {
point.extraText = size + ' ' + DIRSYMBOL.increasing;
point.extraText = formatNumber(size) + ' ' + DIRSYMBOL.increasing;

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 2, 2019

Contributor

Use Axes.hoverLabelText, it's made specifically for this purpose:

pointData2[vLetter + 'Label'] = (t.labels ? t.labels[attr] + ' ' : '') + Axes.hoverLabelText(vAxis, val);

This comment has been minimized.

Copy link
@archmoj

archmoj Apr 2, 2019

Author Collaborator

Thanks for the review @alexcjohnson.
Revised in 07b6c9c.

@etpinard etpinard added the type: bug label Apr 2, 2019

} else {
point[sizeLetter + 'LabelVal'] = size;
point[sizeLetter + 'LabelVal'] = formatNumber(size);

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 2, 2019

Contributor

not quite sure where this would break down, but in principle *LabelVal should be a number, and *Label should be the string you want to use to display that number.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 2, 2019

Contributor

Perhaps non-default separators, or tickprefix/suffix, would break this as is

This comment has been minimized.

Copy link
@archmoj

archmoj Apr 2, 2019

Author Collaborator

In bar traces we use a similar pattern:

pointData[sizeLetter + 'LabelVal'] = size;
.
Do you think there would be a problem there?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 2, 2019

Contributor

Bar uses *LabelVal correctly, it's a number. I'm guessing that if there's no *Label then later on *LabelVal gets converted to a string using hoverLabelText but I don't know. If so, perhaps you can just leave this one as a number too.

Whew, the hover system is certainly ripe for refactoring... or at least documenting.

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 2, 2019

Member

Whew, the hover system is certainly ripe for refactoring... or at least documenting.

🙏

This comment has been minimized.

Copy link
@archmoj

archmoj Apr 2, 2019

Author Collaborator

Bar uses *LabelVal correctly, it's a number. I'm guessing that if there's no *Label then later on *LabelVal gets converted to a string using hoverLabelText but I don't know. If so, perhaps you can just leave this one as a number too.

OK. Let's keep this as it is in this PR.

@etpinard

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Nice fix. Good enough to be released in 1.46.1 💃

@archmoj archmoj merged commit e8b9009 into master Apr 2, 2019

10 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-bundle Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine3 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@archmoj archmoj deleted the fix3710-format-numbers-waterfall-hover branch Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.