[WIP] Fix issue #384 (invisible legends when y is negative) #410
Conversation
| console.error('Legend.draw: insufficient space to draw legend'); | ||
| legend.remove(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
On second thoughts, it'd be more user-friendly to provide a safe default here. For example:
- if
opts.height >= fullLayout.height, thenly = fullLayout.margin.t; scrollHeight = fullLayout.height; - else locate the legend the bottom.
n-riesco
force-pushed
the
issue-384-legend-negative-y
branch
from
2388e2b to
45d5eb1
Compare
* Ensure that when the margins are expended to allocate the legend, there is enough space to clear the layout margins. Fixes plotly#384
* Commented out the statements to round off the legend location, in order to reproduce the baseline images. * There are still 4 image tests that fail: 27, 5, autorange-tozero-rangemode, and range-selector-style. All these mocks locate the legend above the axis (i.e. y > 1).
n-riesco
force-pushed
the
issue-384-legend-negative-y
branch
from
45d5eb1 to
ca23f6c
Compare
|
The location of the plot title seems to be affected by the same issue that here I'm fixing for legends. I've checked and 3 out of the 4 failing mocks (5, 27 and autorange-tozero-rangemode) have a title that overlaps the top margin. I haven't checked yet, but I suspect that the 4th failing mock (range-selector-style) suffers from a similar problem with the bottom margin. |
| // Similar to plots.autoMargin, except that it pads the request with the layout | ||
| // margins, so that an object can be drawn without reaching the layout vertical | ||
| // margins. | ||
| plots.autoMarginVertical = function(gd, id, o) { |
|
I'm not convinced we should add a different auto-margin step to accommodate negative legend positions. As fas as I know, negative legend positions were working fine before #243 (or in #365). It's really a shame that we don't have any image test for those cases. But, it looking back at |
|
@etpinard I've checked v1.5.2 (#243 was introduced after) and the issue is not present. I have a question though. How strict are the margins defined in Shall I remove |
Thanks for the investigation 🍻
No very strict. They need to be greater than zero and not bigger than the plot width and/or height
It's more like the other around: components, like legends and titles expand the margins (by default).
Components are allow to overlap the margins if that's what the users sent as
Yes. You should try to fix this issue from within the |
1 similar comment
Thanks for the investigation 🍻
No very strict. They need to be greater than zero and not bigger than the plot width and/or height
It's more like the other around: components, like legends and titles expand the margins (by default).
Components are allow to overlap the margins if that's what the users sent as
Yes. You should try to fix this issue from within the |
|
Moved to #417 |
TODO
Plot.autoMarginVertical()to something more sensible (any suggestions?)select('rect')pie hovering event data should contain the correct fields FAILEDpie hovering event data should fire when moving from one slice to another FAILEDthe range slider when specified as visible should react to resizing the maximum handle FAILED