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

Fix JSON error when saving notebook with plotly figure #159

Merged
merged 2 commits into from
Sep 20, 2018

Conversation

jonmmease
Copy link
Contributor

@jonmmease jonmmease commented Sep 17, 2018

This PR is a fix for #158

It looks to me that after computing the png image, the model.setData call was treating
plotly's traces array (data) as if it were the bundle's data property (model.data).

This was causing a malformed bundle to be saved in the notebook, and causing only the image/png form to be restored on notebook load.

cc @gnestor

After computing the png image, the `model.setData` call was treating
plotly's traces array (`data`) as if it were the bundle's `data`
property (`model.data`).

This was causing a malformed bundle to be saved in the notebook,
and causing only the image/png form to be restored on notebook load.
When opening a plotly figure from a file the `this.node.offsetWidth`
and `this.node.offsetHeight` values are 0, which causes an error in
`Plotly.toImage`.  Add guards to not call `Plotly.toImage` unless
width and height are positive numbers.
@jonmmease
Copy link
Contributor Author

Added 9f9992a to fix #160.

When opening a plotly figure from a file the this.node.offsetWidth
and this.node.offsetHeight values are 0, which causes an error in
Plotly.toImage. This commit guards to not call Plotly.toImage unless
width and height are positive numbers.

@jonmmease
Copy link
Contributor Author

For context, I'm working on adding support to plotly.py to read/write JSON files that are compatible with this extension. I'm aiming for a release in about a week and I'd love to have these fixes available in a released version of @jupyterlab/plotly-extension so that we could show it off alongside the plotly.py release. Does that sound feasible @blink1073 @ian-r-rose @gnestor ? Thanks!

@blink1073
Copy link
Contributor

LGTM, thanks!

@blink1073 blink1073 merged commit 660d152 into jupyterlab:master Sep 20, 2018
@blink1073
Copy link
Contributor

Published @jupyterlab/plotly-extension@0.17.2

@jonmmease
Copy link
Contributor Author

Awesome, thanks @blink1073 !!

@blink1073
Copy link
Contributor

👊

@dhirschfeld
Copy link
Member

I think I was seeing this exact error today so good to know it's already fixed! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants