search

statgen / locuszoom

A Javascript/d3 embeddable plugin for interactively visualizing statistical genetic data from customizable sources.
https://statgen.github.io/locuszoom/
MIT License
154 stars 29 forks source link

Fix issue with plot resizing when plot or panel min_height is specified #115

Closed abought closed 6 years ago

abought commented 7 years ago

Purpose

Fix issue reported by @benralexander, where in certain situations the plot did not resize as expected when a panel was removed.

The issue was traced to an edge case that occurred when plots and/or panels specified a min_height. Specifically, the plot was "remembering" minimum constraints based on a panel that was no longer there- math.max provided a mechanism for min_height to grow, but not to shrink.

Demonstrations

The issue is demonstrated below. It could be reproduced on many of our core sample plots, including phewas_scatter.

The browser demo in these images also applies proportional height constraints, so the numbers in the example below differ slightly vs unit tests.

Before Removing a panel did not free up as much space as would be expected. Remaining panels stretched to fit a larger plot size. After plot removal, plot.layout.height===667 resize_bug_demo_before

After (if plot min_height is specified) Plot shrinks slightly (to the minimum required dimensions, plot.layout.height===600), but space is reallocated between the remaining panels, so the image appears to stretch and shrink a bit.

resize_bug_demo_after_with_minheight

After (if plot min_height is not specified) Plot shrinks by exactly the size of the removed panel- the user experience is as if the bottom section simply disappeared. plot.layout.height===367 at end) resize_bug_demo_after_no_minheight

Reviewer notes

The resize logic supports a number of configuration options. I have done some basic manual regression testing on other examples, but please let me know if this fix could be problematic.

I intentionally left the behavior in the second screenshot as is- stretching seems a valid response to having more room than any one panel requested, and the user can get the desired appearance by providing panel min_heights (but omitting min_height for the layout of the parent plot).

That said, this is nuanced behavior, so feedback is welcome.

abought commented 7 years ago

Sorry for the reopening spam; it's an old trick for when Travis needs persuading. The fix feels a little hacky, but this is not a sign of extreme ambivalence either. 😉

CI is now working on PRs and the new develop branch.

abought commented 6 years ago

Thanks, Chris! Ben has also confirmed this works in their more complex test case, so this looks good to merge to develop.