-
-
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
Automargin zoom #6312
Automargin zoom #6312
Conversation
@hannahker FYI - your fix does not address this other case that you found here: #5567 (comment) |
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
@archmoj yes, I did realize... there seem to be two (potentially related) issues:
Do you think it's worth addressing both here? |
No I prefer addressing different issues in different PRs. BTW the issue you mentioned is the expected behavior not a bug. So we are good here. |
function assertLayout() { | ||
var titleTop = getPos(gd, '.xtitle').top; | ||
var tickBottom = getPos(gd, '.xtick').bottom; | ||
expect(tickBottom).toBeLessThan(titleTop + 2); // allow two pixels tolerance |
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.
Just to note: this specific test looks fine, but only because:
(1) all the tick labels are essentially the same length. select(...).node()
gives you only the first element, but the first one is unlikely to conflict with the axis title. Only the labels that are directly above the title can push it down.
(2) the tick labels are dense enough that even when you zoom in the title never fits between them. If that's not the case, when the title lines up right it can actually squeeze up between those labels.
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.
💃 Very nice - good find, excellent test!
Co-authored-by: Alex Johnson <alex@plot.ly>
Fix for #5567. This is a single-line fix, although I'll admit to not 100% understanding the logic behind what this line intended to do in the first place -- I can't find any undesired implications of removing this.