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

Automargin zoom #6312

Merged
merged 12 commits into from Sep 15, 2022
Merged

Automargin zoom #6312

merged 12 commits into from Sep 15, 2022

Conversation

hannahker
Copy link
Member

@hannahker hannahker commented Sep 7, 2022

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.

@archmoj
Copy link
Contributor

archmoj commented Sep 8, 2022

@hannahker FYI - your fix does not address this other case that you found here: #5567 (comment)

hannahker and others added 2 commits September 8, 2022 16:44
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
@hannahker
Copy link
Member Author

hannahker commented Sep 8, 2022

@archmoj yes, I did realize... there seem to be two (potentially related) issues:

  1. in some cases the margin isn't big enough to avoid overlap with an axis title
  2. zooming in to the plot causes an incorrect redraw of the axis title

Do you think it's worth addressing both here?

@hannahker
Copy link
Member Author

Ah I think 1) is happening when the size of the plot can't be shrunk anymore because because it's reached the default minreducedwidth of 64px. The only thing I can think of here would be to reduce the default to something smaller.

Screen Shot 2022-09-08 at 5 03 27 PM

@archmoj
Copy link
Contributor

archmoj commented Sep 8, 2022

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
Copy link
Contributor

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.

Copy link
Contributor

@alexcjohnson alexcjohnson left a 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>
@archmoj archmoj merged commit 1f8b47c into master Sep 15, 2022
@archmoj archmoj deleted the automargin-zoom branch September 15, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants