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

Adjust minfinalheight in automargin routine to allow narrow subplots e.g. gauge #5315

Merged
merged 6 commits into from Dec 4, 2020

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Dec 4, 2020

Fixes #5314.

@plotly/plotly_js

 - reduce minfinalheight to 16
 - revert jasmine tests to what we had before commit 565f942
@@ -1866,8 +1866,8 @@ function initMargins(fullLayout) {
if(!fullLayout._pushmarginIds) fullLayout._pushmarginIds = {};
}

var minFinalWidth = 64; // could possibly be exposed as layout.margin.minfinalwidth
var minFinalHeight = 64; // could possibly be exposed as layout.margin.minfinalheight
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should define two values for each dimension here (so repeat this for width as well):

// non-negotiable - this is the smallest height we will allow users to specify via explicit margins
var MIN_SPECIFIED_HEIGHT = 2;

// could be exposed as an option - the smallest we will allow automargin to shrink a larger plot
var MIN_REDUCED_HEIGHT = 64;

Then down below we can combine these two:

var minFinalHeight = Lib.constrain(
    height - margin.t - margin.b,
    MIN_SPECIFIED_HEIGHT,
    MIN_REDUCED_HEIGHT
);

The goal is:

  • if you specify a plot height larger than MIN_REDUCED_HEIGHT, automargin is allowed to increase the top and bottom margins to reduce the plot height as low as MIN_REDUCED_HEIGHT.
  • if you specify a plot area smaller than MIN_REDUCED_HEIGHT but larger than MIN_SPECIFIED_HEIGHT, automargin will make no changes.
  • if you specify a plot height smaller than MIN_SPECIFIED_HEIGHT, automargin should reduce the margins symmetrically to increase the plot height back to MIN_SPECIFIED_HEIGHT

@archmoj archmoj changed the title Adjust minfinalheight in automargin routine to allow narrow height subplots e.g. gauge Adjust minfinalheight in automargin routine to allow narrow subplots e.g. gauge Dec 4, 2020
Copy link
Collaborator

@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.

Beautiful. 💃

@archmoj archmoj merged commit 6cf9f32 into master Dec 4, 2020
@archmoj archmoj deleted the fix5314-reduce-minfinalheight branch December 4, 2020 16:20
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.

Plot sizing issue in 1.58.0 release
2 participants