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

add support for responsive charts #2974

Merged
merged 8 commits into from
Sep 7, 2018
Merged

add support for responsive charts #2974

merged 8 commits into from
Sep 7, 2018

Conversation

antoinerg
Copy link
Contributor

Initial implementation of responsive charts as proposed in #2969.

By setting {responsive:true} in a plot's config, it will be automatically resized when the window size changes. For now it only reacts to window's resize but could be improved by using ResizeObserver down the road.

Viewing a responsive chart on a phone going from landscape to portrait now results in the following experience:
after

`responsive: true` will trigger Plotly.resize whenver the window is resized

// Listen to window resize
window.addEventListener('resize', gd._responsiveChartHandler);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code looks great, but I'm a little surprised to see it so late in the sequence... resize has an internal delay, and calls Plotly.relayout which will wait for any pending drawing on the plot to finish before making its changes. So to avoid missing events that happen halfway through an async draw (mathjax or maps) I'd have thought we should put this in the synchronous block.

Copy link
Contributor Author

@antoinerg antoinerg Sep 5, 2018

Choose a reason for hiding this comment

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

It doesn't have to be this late. To be honest, I wasn't sure where it'd be best to put it in the sequence. Should I put it at line 193?

for(var i = 0; i < gd.calcdata.length; i++) {
gd.calcdata[i][0].trace = gd._fullData[i];
}
/*
* start async-friendly code - now we're actually drawing things
*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I put it at line 193?

Seems like a good place!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh one more thing occurs to me: a user could in principle start with responsive and then turn it off using Plotly.react... not sure why you'd want to do this, but it should work!

// remove responsive handler
if(gd._responsiveChartHandler) {
window.removeEventListener('resize', gd._responsiveChartHandler);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to delete gd._responsiveChartHandler so it would reattach if we make a new plot in this div later on. The rest of this happens in Plots.purge (called a few lines down), and the delete and window.removeEventListener need to be in the same place (since there are a couple of places we call Plots.purge outside Plotly.purge, so seems like it would be safer to put both in Plots.purge

@alexcjohnson
Copy link
Collaborator

@antoinerg thanks for pushing on this one, it's looking good!

I'm not sure how feasible it is to write a test for my async comment - you could try resizing synchronously after Plotly.plot returns but that might not do it (or might not do it consistently). I guess perhaps we could artificially inject a delayed promise into gd._promises but... might be too hacky to be worth it.

But the comment about purging and replotting we can definitely test, also lets test that we have exactly one resize handler and one redraw after making a plot, then editing something (causing a new Plotly.plot call), and THEN resizing the window. That one looks like it already works, lets keep it that way 😈

- move cleanup code into Plots.purge
- attach responsive handlers synchronously earlier in the drawing sequence
- honors Plotly.react
- add several tests
checkLayoutSize(newWidth, newHeight);
})
.catch(failTest);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea to pull this function out! Nice and 🌴. You can probably simplify it a bit though:

function testResponsive() {
    checkLayoutSize(startWidth, startHeight);
    viewport.set(newWidth, newHeight);

    return delay(200)()
    .then(function() {
        checkLayoutSize(newWidth, newHeight);
    })
    .catch(failTest);
}

And use it like:

Plotly.plot(gd, data, {}, {responsive: true})
.then(testResponsive)
.then(done);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call :) Thank you again

it('should still be responsive if the plot is redrawn', function(done) {
var promise = Plotly.plot(gd, data, {}, {responsive: true})
.then(function() {
Plotly.restyle(gd, 'y[0]', data[0].y[0] * 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to return these Plotly.* calls inside .then - as with any function that itself returns a promise - so the new promise gets added to the chain.

@@ -2234,6 +2243,9 @@ exports.react = function(gd, data, layout, config) {
gd._context = undefined;
setPlotContext(gd, config);
configChanged = diffConfig(oldConfig, gd._context);

// remove responsive handler
Lib.clearResponsive(gd);
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm actually there's another way to turn off config.responsive on a div, by calling newPlot again. I think the removal belongs in Plotly.plot instead, next to where you add the handler:

if(gd._context.responsive) {
    if(!gd. _responsiveChartHandler) { ...attach }
}
else {
    Lib.clearResponsive(gd);
}

Then it would handle both cases.

@alexcjohnson
Copy link
Collaborator

Alright, looks great now - thanks for the quick turnaround on my comments! 💃

@etpinard
Copy link
Contributor

etpinard commented Sep 7, 2018

@antoinerg's 1st feature! 🍾

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.

None yet

3 participants