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

Fix hover label exponents #1932

Merged
merged 3 commits into from
Aug 5, 2017
Merged

Fix hover label exponents #1932

merged 3 commits into from
Aug 5, 2017

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented Aug 4, 2017

This PR fixes #755 and #1711. After a bit of digging:

The problem: Hover labels are sometimes wrong by arbitrarily gigantic orders of magnitude, which is bad. Hover labels piggyback on Axes.tickText, which sometimes hides exponents and just shows 1/2/5 values. That's always invalid for hover labels since their exponent is not shown elsewhere as it is for axes.

The solution: whenever 'hover' is passed to tickText, this PR prevents it from making any automated decision about whether to hide the exponent, instead just obeying the axis formatting.

@alexcjohnson
Copy link
Collaborator

Looks good to me. I could imagine with showexponent: 'none' wanting the hover labels to have no exponents either - but that's a weird case and I could equally imagine wanting the exponents only on hover in that case. If you want that kind of control you can make your own hover text :)

This whole system of formatLinear/formatLog/numFormat is a bit of a palimpsest at this point and probably should be completely refactored... but for now this is great.

I took the liberty of merging #1930 first, so you can fix these tests 🎉 and then 💃

Unverified

This user has not yet uploaded their public signing key.
@rreusser rreusser force-pushed the fix-hover-exponents branch from 418e85e to 08cfd4a Compare August 5, 2017 04:29
@rreusser
Copy link
Contributor Author

rreusser commented Aug 5, 2017

Haha good word. Yeah, the logic was about a 6/10 for subtle and intricate, so I was a bit worried I'd miss some cases. As is often the case then, I focused mainly on fixing the bug while adding and modifying as little as possible. I don't quite understand the above comment about showexponent: 'none' (like 0.0000000000000000112 then?) but seems like the top priority is that they're not wrong. Opting for merge. 😄

@rreusser rreusser merged commit 4e86a36 into master Aug 5, 2017
@rreusser rreusser deleted the fix-hover-exponents branch August 5, 2017 04:43
// it's now succeeded by preventing the other condition from automating
// this choice. Thus we can unset it so that the axis formatting takes
// precedence.
hideexp = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the 📚 @rreusser

@etpinard
Copy link
Contributor

etpinard commented Aug 7, 2017

Very nice PR @rreusser thanks for doing this 🎉

🔪'ing two issues in ~10 new LOCs is amazing!

@etpinard
Copy link
Contributor

etpinard commented Aug 8, 2017

@rreusser this PR breaks this gl3d hover label test. CI ✅ as this particular test is skipped on CI.

image

Can you make sure npm run test-jasmine -- gl_plot_interact passes?

@rreusser
Copy link
Contributor Author

rreusser commented Aug 8, 2017

Sure. En route to meeting with Alex today so will fix during the flight, along with the final trace naming issue.

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.

Hover label incorrect when number is very small (or large?)
3 participants