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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve autosize in edge cases #3090

Merged
merged 2 commits into from Oct 10, 2018

Conversation

Projects
None yet
2 participants
@antoinerg
Copy link
Collaborator

commented Oct 10, 2018

In this PR, I address some shortcomings of #3056 by doing the following things:

  • provide fixed non-zero width/height when autosize:true and container's size is zero
  • when setting container's widht/height, ignore autosize:true if width/height is explicitly specified by the user.
  • 馃敀 down in test that explicit width/height wins over autosize:true
improve autosize in edge case
- provide fixed non-zero width/height when autosize:true and container's size is zero
- when setting container's widht/height, ignore autosize:true if width/height is explicitly specified by the user.
- 馃敀 down in test that explicit width/height win over autosize:true
@@ -488,6 +488,10 @@ function setPlotContext(gd, config) {
if(context.setBackground === 'transparent' || typeof context.setBackground !== 'function') {
context.setBackground = setBackground;
}

// Check if gd has a specified widht/height to begin with
context._hasZeroHeight = context._hasZeroHeight || gd.clientHeight === 0;

This comment has been minimized.

Copy link
@etpinard

etpinard Oct 10, 2018

Member

Why are we stashing things on _context now?

@@ -3248,9 +3252,6 @@ function makePlotFramework(gd) {
var gd3 = d3.select(gd);
var fullLayout = gd._fullLayout;

// Check if gd has a specified height
fullLayout._hasZeroHeight = gd.clientHeight === 0;

This comment has been minimized.

Copy link
@antoinerg

antoinerg Oct 10, 2018

Author Collaborator

The reason I removed _hasZeroHeight from fullLayout and put it into context is because it would get cleared by a call to Plotly.newPlot. However, because this call does not remove the plot from the container, gd.clientHeight would be non-zero leading us to believe the container has a specified width/height in CSS. This is not the case: it simply expanded to accomodate the plot. As soon as the plot is removed, it goes back to being zero size which messes things up.

This comment has been minimized.

Copy link
@etpinard

etpinard Oct 10, 2018

Member

Ok. That looks good to me. Stashing those things _context is "great", but I can't think of a better place, so 馃憣

The problems brought up in #3056 (comment) seemed to now be resolved.

I'll test this thing on stage after lunch.

width: (fullLayout.autosize) ? '100%' : fullLayout.width + 'px',
height: (fullLayout.autosize && !fullLayout._hasZeroHeight) ? '100%' : fullLayout.height + 'px'
width: (fullLayout.autosize && !gd._context._hasZeroWidth && !gd.layout.width) ? '100%' : fullLayout.width + 'px',
height: (fullLayout.autosize && !gd._context._hasZeroHeight && !gd.layout.height) ? '100%' : fullLayout.height + 'px'

This comment has been minimized.

Copy link
@antoinerg

antoinerg Oct 10, 2018

Author Collaborator

The && !gd.layout.width and && !gd.layout.height conditions are there to ensure that explicit width/height wins over autosize:true.

This comment has been minimized.

Copy link
@antoinerg

antoinerg Oct 10, 2018

Author Collaborator

I just realized that one way of making sure we're not breaking anything out there would be to add a && gd._context.responsive. That way, we would absolutely have no regression for people not using responsive! After all, setting the container's size to 100% is only useful for responsiveness in modern CSS layouts.

This comment has been minimized.

Copy link
@etpinard

etpinard Oct 10, 2018

Member

I just realized that one way of making sure we're not breaking anything out there would be to add a && gd._context.responsive

馃憤

only set container's size to 100% when responsive:true
This change will prevent any existing applications from breaking. Anyway setting the container's size to 100% was only useful for responsiveness in modern CSS layouts.
@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Nicely done. 馃拑

Thanks for the quick turnaround!

@antoinerg antoinerg merged commit cc21e5c into master Oct 10, 2018

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@antoinerg antoinerg deleted the fix-autosize branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.