-
Notifications
You must be signed in to change notification settings - Fork 569
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
#636 fixed widget display for both formats #778
Conversation
function addWidgetsRenderer() { | ||
var mimeElement = document.querySelector('script[type="application/vnd.jupyter.widget-view+json"]'); | ||
var scriptElement = document.createElement('script'); | ||
var widgetRendererSrc = 'https://unpkg.com/@jupyter-widgets/html-manager@*/dist/embed-amd.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this always be unpinned, or should we be picking a version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is identical to nbviewer and it's been working well there: https://github.com/jupyter/nbviewer/blob/67ee47e55fa697614e03aa060a86019f75f2ddca/nbviewer/templates/notebook.html#L90
and that code was taken from jason's documentation:
http://ipywidgets.readthedocs.io/en/stable/migration_guides.html?highlight=migrate#updating-embedded-widgets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some comments to explain the reasoning behind it this entire function? Also please include a pointer to jason's documentation in the comments.
Could you add a test for this new functionality? |
I am not sure if we can add meaningfull unit tests for this change. When html output is generated it has js function that will load propper script according to used notebook/widget version. To check if this work correct we need to evaluate java script code and check if widget was displayed properly after that. |
Closing as fixed in #792. |
Fix #636 using the same approach as we used on jupyter/nbviewer#717