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

responsive figures in CSS flexbox and grid #3056

Merged
merged 5 commits into from Oct 5, 2018

Conversation

Projects
None yet
3 participants
@antoinerg
Copy link
Collaborator

commented Sep 27, 2018

Fixes #3034

Modern CSS layouts are able to grow and shrink children according to the leftover space. The leftover space is the space to fill minus the base sizes of the non-flexible grid tracks. Because the svg-container gets a fixed width and height in pixels after being created, the plot becomes non-flexible and would never give space back. This resulted in the plot not resizing when the window was made smaller.

var gdBB = {
width: fullLayout.width,
height: fullLayout.height
};

This comment has been minimized.

Copy link
@antoinerg

antoinerg Sep 27, 2018

Author Collaborator

@alexcjohnson @etpinard I changed this part in order to pass the test saying it should fill the container when autosize: true, fillFrame: false and frameMargins: 0.1. Because I only 🔪 code, I'm worried that this was solving an edge case which is not properly tested as of right now. What do you think?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Sep 28, 2018

Contributor

Looks to me as though all of this dates to the toolpanel era (but not the toolpanel itself, AFAICT)... the only place I see a nonzero frameMargins is in super-old streambed code and the only place I see gd._boundingBoxMargins is embedded in a coverage test I apparently ran in 2015. So I'm tempted to say we should 🔪 this whole section but that's probably not a good idea. We should certainly 🔪 calculateReservedMargins and gd._boundingBoxMargins

Lets try to fold frameMargins into the block below (// plotly.js - let the developers do what they want) and see if we can make something meaningful out of that. I think if you get rid of the getBoundingClientRect, that was intended to be the primary path so we're losing the purpose here - which seems to be to size the plot to some fixed percentage smaller than its container. Below, the getBoundingClientRect was replaced by window.getComputedStyle in order to support more cases, so if we're going to keep frameMargins it should benefit from that as well.

This comment has been minimized.

Copy link
@etpinard

etpinard Sep 28, 2018

Member

b3a4f23 looks great. Thanks @antoinerg !

@etpinard etpinard added this to the v1.42.0 milestone Sep 28, 2018

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

Looks great @antoinerg! The new and updated tests are very good, but I think it's still a good idea to test this manually in streambed before we merge - check that the 3 key plot environments we have there work as they do now: create, shareplot, and embedplot.

@etpinard

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

check that the 3 key plot environments we have there work as they do now: create, shareplot, and embedplot.

Here are some (maybe outdated) docs about on how to that:

https://github.com/plotly/dev-docs/blob/master/advanced/plotly.js.md#developing-plotlyjs-in-streambed

@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

but I think it's still a good idea to test this manually in streambed before we merge

I'll take this on tomorrow. I'd love to get this into 1.42.0.

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 4, 2018

I started to look into it but got discouraged at getting streambed up and running under Windows. Thank you @etpinard if you can test things out!

@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Everything looks good in shareplot, embedplot and create on stage, but two embedplot tests are failing:

image

@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

but can be fixed with:

image

which appears equivalent.

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 5, 2018

@etpinard Could you visually inspect the result to make sure it's really the same?

Thanks so much for this BTW 👏👏👏

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

Excellent, thanks for testing that out @etpinard - lets do it! 💃

@antoinerg antoinerg merged commit 9f5733d into master Oct 5, 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 3034-responsive-flexbox branch Oct 5, 2018

@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Hmm. I think this PR "broke' the plotly-mock-viewer.

Entering

Plotly.newPlot(gd, [{y: [1,2,1]}])

in the console gives

peek 2018-10-09 14-44

@antoinerg Is this the new expected behaviour, or something to be concerned of?

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 10, 2018

@etpinard The issue right now with the plotly-mock-viewer is due to the fact that it uses a container with display: inline-block. Contrarily to a display: block, it doesn't fill its parent horizontally so the graph container ends up with a width which spans 100% of nothing = 0px.

To make it work as it used to, I will change the following line

width: (fullLayout.autosize) ? '100%' : fullLayout.width + 'px',

to something like width: (fullLayout.autosize && !fullLayout._hasZeroWidth ) ? '100%' : fullLayout.width + 'px',
similarly to what I've done for the height:
height: (fullLayout.autosize && !fullLayout._hasZeroHeight) ? '100%' : fullLayout.height + 'px'

This should fix the issue. I will write a test to make sure that it does and that we get correct results for the various CSS display options.

@alexcjohnson suggested I open a new PR so I will do that tomorrow!

@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

suggested I open a new PR so I will do that tomorrow!

Yes, that would be great!

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