responsive figures in CSS flexbox and grid

[antoinerg]

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.

[antoinerg]

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

[alexcjohnson]

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.

[etpinard]

b3a4f23 looks great. Thanks @antoinerg !

[alexcjohnson]

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]

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]

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]

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]

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

[etpinard]

but can be fixed with:

which appears equivalent.

[antoinerg]

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

Thanks so much for this BTW 👏👏👏

[alexcjohnson]

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

[etpinard]

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

Entering

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

in the console gives

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

[antoinerg]

@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

plotly.js/src/plot_api/subroutines.js

Line 57 in 202f8f9

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:

plotly.js/src/plot_api/subroutines.js

Line 58 in 202f8f9

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]

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

Yes, that would be great!