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 handler: do not resize if gd is hidden #3972

Merged
merged 3 commits into from
Jun 25, 2019
Merged

Conversation

antoinerg
Copy link
Contributor

Fixes #3866 using approach number 2 from comment #3866 (comment)

function isHidden(gd) {
var display = window.getComputedStyle(gd).display;
return !display || display === 'none';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the same helpers as in Plots.resize?

Can we make this a Lib function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea!

Done in 913bb68

// make the figure responsive
if(gd._context.responsive) {
if(!gd._responsiveChartHandler) {
// Keep a reference to the resize handler to purge it down the road
gd._responsiveChartHandler = function() { Plots.resize(gd); };
gd._responsiveChartHandler = function() { if(!isHidden(gd)) Plots.resize(gd).catch(Lib.noop); };
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to .catch here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't! The whole point is to not ignore possible rejected promises. Thanks, @etpinard for noticing this.

Fixed in 913bb68

@etpinard etpinard added status: reviewable bug something broken labels Jun 18, 2019
@etpinard etpinard added this to the v1.49.0 milestone Jun 18, 2019
@etpinard
Copy link
Contributor

Thanks for taking this on @antoinerg !

Pinning this for v1.49.0

@etpinard
Copy link
Contributor

thanks again for doing this @antoinerg 💃

@etpinard
Copy link
Contributor

(let's start merging things for 1.49.0)

@antoinerg antoinerg merged commit 705b184 into master Jun 25, 2019
@antoinerg antoinerg deleted the fix-3866 branch June 25, 2019 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Resize on hidden graph divs causes Error
2 participants