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

Jupyter Notebook: Fix window.Plotly not defined error when running all cells #420

Merged
merged 1 commit into from
Apr 11, 2016

Conversation

dalogsdon
Copy link
Contributor

In Dynamic Dashboards (jupyter/dashboards_server#108) we need to be able to execute all code cells of a Jupyter Notebook file. Currently Plotly does not work, with the error that window.Plotly is not defined. This problem can also be observed in a notebook when performing a "Run All" of all cells.

@jdfreder's fix in #412 preserves requirejs when plotting in offline mode in the notebook, but causes an error if doing a "Run All" cells. The reason for this is the initialization of window.Plotly occurs in a requirejs function and is immediately followed by an attempt to access it outside of a requirejs function. At this point window.Plotly won't yet be defined since the definition will execute asynchronously after the attempt to access it.

To fix, the call to Plotly.newPlot() could happen inside its own requrejs function, making use of the 'plotly' requirejs module. I have made the minimal change in this PR to demonstrate. (I kept the requirejs function that defines window.Plotly, but it could potentially be removed in favor of using the requirejs module only.) It works for me when doing a "Run All" in the notebook and also works when deploying a dashboard read from the notebook file.

This may also be related to #394 if the conversion of notebook to slides is doing a "Run All".

@jdfreder
Copy link
Contributor

jdfreder commented Mar 9, 2016

Thanks @dalogsdon , 👍

@theengineear
Copy link
Contributor

@jdfreder , does this need to wait on any dev you're doing, or can we merge this in now?

@jdfreder
Copy link
Contributor

@theengineear thanks for asking, but no, it doesn't, go ahead and merge away :)

@theengineear
Copy link
Contributor

great, thx!

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