Skip to content

Provide room for large computed margins #5237

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 4, 2020
Merged

Provide room for large computed margins #5237

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
404fe7a
fix issue 4459 - provide more room to bring back large computed margins
archmoj Oct 27, 2020
09e2339
add image test
archmoj Oct 28, 2020
921e5a5
add a test for the case of large horizontal margin
archmoj Oct 28, 2020
28fc880
add a minimum pixel for margins - add test with margins on both sides
archmoj Oct 28, 2020
670d238
reduce large margins on both sides by a ratio instead of setting them…
archmoj Nov 2, 2020
565f942
adjust large margins instead of dropping them
archmoj Nov 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1900,11 +1900,11 @@ plots.autoMargin = function(gd, id, o) {

// if the item is too big, just give it enough automargin to
// make sure you can still grab it and bring it back
if(o.l + o.r > fullLayout.width * 0.5) {
if(o.l + o.r > fullLayout.width) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean your automargin can expand so much the plot itself completely disappears. That doesn't seem like a good idea. I'm also concerned about what would happen if you have large margin pushers on opposite sides - tick labels on the left and legend on the right, for example. I suspect that with the 0.5 that was here before you could in principle get right up to the edge of no visible plot but not quite pass it.

Perhaps rather than discarding the constraint, a better solution would be to just limit the end result?

  • First constrain each individual value here (o.l, o.r etc) to no more than half the total size, which I think should ensure there IS a solution to the loop trying to satisfy all these constraints
  • Then once we have candidate margins here we can reduce them as necessary to ensure there's still some visible plot area. Perhaps don't allow the plot area to be smaller than 1/4 the total size? And if a reduction is needed, decrease both margins by the same factor as long as neither becomes less than the input layout.margin value - in that case take more from the other side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson Thanks for the review and pointers.
I added an image test in 09e2339.
Noting that in this example we wanted the lower margin to be greater than half of the size, I still think that to fix this bug we should relax the constraint there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m OK with allowing one margin to be bigger than half the plot, as long as (a) there are no situations where we fail entirely (large margins on both sides from different elements - this will need a test or two) and (b) the plot never becomes too small to be useful (I suggest minimum 1/4 the total size but we could go a little further if you think it important)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(b) the plot never becomes too small to be useful (I suggest minimum 1/4 the total size but we could go a little further if you think it important)
I can imagine of cases where one wants to dedicate 7/8 (or even more) of the plot area to the labels and the graph still look nicely on the remaining 1/8 part.
How about declaring the minimum in pixels? Something like 100px for now which could also be configured later as minwidth & minheight in layout or margin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(a) there are no situations where we fail entirely (large margins on both sides from different elements - this will need a test or two)

While testing this with margins on both sides I didn't encounter a major change between before and after behaviour in respect to this change.
In fact the change here basically allows one margin to be bigger than half of the plot not both which is the reported bug.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea - layout.margin.minfinal(width|height) perhaps, with a default of something like 100 (px).

Copy link
Contributor Author

@archmoj archmoj Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Addressed in 28fc880.
And we could expose those constants (also with a relative option) in a separate PR.

Lib.log('Margin push', id, 'is too big in x, dropping');
o.l = o.r = 0;
}
if(o.b + o.t > fullLayout.height * 0.5) {
if(o.b + o.t > fullLayout.height) {
Lib.log('Margin push', id, 'is too big in y, dropping');
o.b = o.t = 0;
}
Expand Down