fix responsive figures with WebGL #3486
Conversation
| @@ -244,8 +244,6 @@ exports.plot = function(gd, data, layout, config) { | |||
| 'position': 'absolute', | |||
| 'top': 0, | |||
| 'left': 0, | |||
| 'width': '100%', | |||
| 'height': '100%', | |||
There was a problem hiding this comment.
I don't think those two lines were ever needed since a few lines later we set the size of the canvas via its attributes anyway:
plotly.js/src/plot_api/plot_api.js
Lines 253 to 255 in f8c959d
| .then(testResponsive) | ||
| .then(done); | ||
| }); | ||
| it('@flaky should resize when the viewport width/height changes', function(done) { |
There was a problem hiding this comment.
We should add the traceType value inside these its so that if the scatter or the scattergl cases fail, we'll know which one did. For example,
it('@flaky should resize when the viewport width/height changes (case ' + traceType + ')', function(done) {
// ...
| document.body.removeChild(parent); | ||
| viewport.reset(); | ||
| }); | ||
| ['scatter', 'scattergl'].forEach(function(traceType) { |
There was a problem hiding this comment.
We should add the traceType value inside these its so that if the scatter or the scattergl cases fail, we'll know which one did. For example,
I think this line allows to clearly identify which case fails. Am I mistaken?
cc @etpinard
There was a problem hiding this comment.
Nop, that won't appear in the logs. Try it!
There was a problem hiding this comment.
Running npm run test-jasmine -- config will a failing test gives me:
What am I missing?
Thank you @etpinard
There was a problem hiding this comment.
Maybe it's not prominent enough but it's already there (scattergl is underlined in yellow):
There was a problem hiding this comment.
Oops. It's in the describe block:
describe('responsive ' + traceType + ' trace', function() {
My bad.
There was a problem hiding this comment.
My bad also: I should have highlighted this describe line for clarity. Anyway, thanks for the review :)
|
Codepen showing the fix: https://codepen.io/anon/pen/NoxBRy Note that the modebar is also misbehaving: it does not fill vertical space properly. The problem is similar to the one affecting canvas: modebar has |
Thanks for the info. Are you planning on trying to fix that in this PR? |
|
@etpinard Yes, I think I should clean up that CSS and fix this here. |
|
In commit 751f716, I move the CSS directive @plotly/plotly_js |
| @@ -105,7 +105,8 @@ exports.plot = function(gd, data, layout, config) { | |||
|
|
|||
| // hook class for plots main container (in case of plotly.js | |||
| // this won't be #embedded-graph or .js-tab-contents) | |||
| d3.select(gd).classed('js-plotly-plot', true); | |||
| d3.select(gd).classed('js-plotly-plot', true) | |||
| .style('position', 'relative'); | |||
There was a problem hiding this comment.
Hmm. I'll call that a breaking change.
We could turn this on just when responsive is set to true. But then again, are there any other solutions?
There was a problem hiding this comment.
Yes, that's certainly a breaking change and it breaks a lot of tests anyway.
I have another branch in which I fix the responsive issues for both WebGL #3485 and the modebar #3499 that doesn't rely on modifying the parent. I will submit a new separate PR with a cleaner commit history and I will close the current one.
|
Closing in favor of #3486 |
Closes #3485
Commit 16e1f4f simply adds
scatterglin the responsive test suite and fails. Commit f8c959d makes the tests pass.