-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
Looks good to me. I could imagine with This whole system of I took the liberty of merging #1930 first, so you can fix these tests 🎉 and then 💃 |
418e85e
to
08cfd4a
Compare
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 |
// 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 = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the 📚 @rreusser
Very nice PR @rreusser thanks for doing this 🎉 🔪'ing two issues in ~10 new LOCs is amazing! |
Sure. En route to meeting with Alex today so will fix during the flight, along with the final trace naming issue. |
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 totickText
, this PR prevents it from making any automated decision about whether to hide the exponent, instead just obeying the axis formatting.