-
-
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
Fixup rangebreaks l2p and p2l functions #4699
Conversation
6a576f1
to
4c5d245
Compare
What's going on with autorange in axes_breaks-night_autorange-reversed.png ? Both the orange and red subplots prior to this PR have tight clipping on one side, and after this PR the tight clipping is on the other side. Neither side should have tight clipping, they should both get 5% padding. Also what's with the label |
src/plots/cartesian/set_convert.js
Outdated
else { | ||
// when falls into break, pick 'closest' offset | ||
q = pos > (min + max) / 2 ? nextI : i; | ||
q = signAx * pos > signAx * (min + max) / 2 ? nextI : i; |
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.
l2p
is a hot path, so it's worth optimizing a good deal - even if it means some duplicated code, though I'm not sure if that would help here, and even if the code needs more comments and is less self-documenting. For example:
signAx * pos
could be done once up front, andvar min = signAx * brk.min
etc would simplify these conditionals a bitfirst
andlast
look unnecessary- Flip
_m2
for y axes after using it to calculate_B
, so we don't need the(isY ? -1 : 1) *
down below. - Move
var isY = axLetter === 'y'
to the outer scope, whereaxLetter
was defined.
p2l
I think is generally not as hot, though maybe for some traces it's used in hover? Not sure. (Note that c2p
uses l2p
and p2c
uses p2l
)
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.
Good calls. Addressed in the commits below.
Fixed by 749430d. |
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.
Excellent work! 💃
Following of #4614,
this PR fixes
l2p
&p2l
functionscorrect reversed ranges fixes rangebreaks mapping on reversed ranges #4700
see 11c8ea2?short_path=563a470#diff-563a470dab197f9857d9545f11805d89
find correct hover indices and fixes rangebreaks hovermode=closest offset #4693
Hover demo: before vs after
Hover demo2: bar + reversed range
@plotly/plotly_js