Handle case of nonnegative or tozero rangemodes for inside ticklabels - #5331
Conversation
| if(ax.rangemode !== 'nonnegative') { | ||
| A = padInsideLabelsOnAnchorAxis(ax, max); | ||
|
|
||
| if(anchorAxis.rangemode !== 'nonnegative') { |
There was a problem hiding this comment.
why would rangemode of anchorAxis be relevant?
There was a problem hiding this comment.
Wait. I had to revert the commit.
| if(!isLinked(fullLayout, ax._id)) { | ||
| A = padInsideLabelsOnAnchorAxis(ax, max); | ||
| B = padInsideLabelsOnThisAxis(ax, max); | ||
| if(ax.rangemode !== 'nonnegative') { |
There was a problem hiding this comment.
I don't think this is really the condition we want - we're not trying to completely prevent extra padding for labels when rangemode is 'nonnegative', we just want to prevent the range from going past zero. ie we never want this to prevent the high end of the range from expanding to make room for the labels, nor do we want to prevent the low end from expanding unless it's going to go past zero - so for example if you have for example scatter points from 1 to 10 and there are inside labels on the low end, so that without labels the range would be something like [0.4, 10.6], the labels should expand the range down to zero but not beyond.
| if( | ||
| anchorAxis.rangemode !== 'tozero' && | ||
| anchorAxis.rangemode !== 'nonnegative' | ||
| ) { |
There was a problem hiding this comment.
I don't think we want any of these rangemode conditions, now that you have nopad in the fallback conditions above. Basically in your new ticklabelposition-5 mock, only the blue one should have labels overlapping bars. Maybe you want to flip either the red or green labels to the bottom, in which case those would overlap the bars too, but otherwise just the blue should hit this condition.
There was a problem hiding this comment.
I removed one of the checks in 0b254e7 but kept the other one.
There was a problem hiding this comment.
The test images look good now, but I don't understand what the remaining check is doing - can you explain?
There was a problem hiding this comment.
I drop the second check as you suggested.
After further investigation, it looks now that we could possibly drop padInsideLabelsOnThisAxis function which used to tweaks the range for giving the start/end labels more chance to appear on the graph. But I'd rather trying that in a separate PR.
archmoj
changed the title
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
Fantastic. Very nice test images! 💃
2 participants
@plotly/plotly_js