Provide room for large computed margins #5237
Conversation
src/plots/plots.js
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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.retc) 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.marginvalue - in that case take more from the other side.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
(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 asminwidth&minheightinlayoutormargin?
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
I like that idea - layout.margin.minfinal(width|height) perhaps, with a default of something like 100 (px).
There was a problem hiding this comment.
Great! Addressed in 28fc880.
And we could expose those constants (also with a relative option) in a separate PR.
archmoj
force-pushed
the
fix4459-labels-disappear
branch
from
b21d36c to
28fc880
Compare
archmoj
changed the title
|
I like the idea of this new |
Thanks for the feedback. We could expose this as a new attribute in a separate PR after fixing this bug. |
|
For reference, pasting slack discussion leading to 565f942 commit:
|
Fixes #4459 and fixes #4835.
@plotly/plotly_js