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.

Sign up for GitHub

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

Jump to bottom

Fix embed resolution issues #1428

Merged
merged 8 commits into from Jun 20, 2017
Merged

Fix embed resolution issues #1428

merged 8 commits into from Jun 20, 2017

Conversation

pbugnion
Copy link
Member

As noted in issue #1427 , the htmlmanager on master suffers from the following issues:

  • it goes to unpkg to fetch widgets in @jupyter-widgets/base. Since it already depends on @jupyter-widgets/base, it might as well just provide the widgets directly (like it does with widgets in @jupyter-widgets/controls.
  • the embed link is stale.

This PR tries to address these issues, but:

  • at the moment, we need to depend on @jupyter-widgets/controls master, not the released version. The fix for this is to cut a new release of /controls.
  • the script tag for the htmlmanager that we put in the Widgets > Embed widgets modal points to the latest version of the htmlmanager, instead of a fixed version. We could hard-code the version we want in embed_widgets.js?

@jasongrout jasongrout added this to the 7.0 milestone Jun 19, 2017
jasongrout
jasongrout reviewed Jun 19, 2017
View reviewed changes
jupyter-widgets-htmlmanager/src/htmlmanager.ts Outdated
@@ -45,7 +49,7 @@ class HTMLManager extends widgets.ManagerBase<HTMLElement> {
*/
protected loadClass(className: string, moduleName: string, moduleVersion: string) {
return new Promise(function(resolve, reject) {
if (moduleName === '@jupyter-widgets/controls') {
if (coreWidgetModules.indexOf(moduleName) >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

We should provide the widgets in separate cases, as in https://github.com/jupyter-widgets/ipywidgets/blob/master/widgetsnbextension/src/manager.js#L97 (and import the base widgets as a separate namespace).

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. Thanks.

jasongrout
jasongrout reviewed Jun 19, 2017
View reviewed changes
jupyter-widgets-htmlmanager/package.json Outdated
@@ -30,7 +30,7 @@
"dependencies": {
"@phosphor/widgets": "^1.2.0",
"font-awesome": "^4.7.0",
"@jupyter-widgets/controls": "^0.1.0",
"@jupyter-widgets/controls": "file:../jupyter-widgets-controls",
Copy link
Member

Choose a reason for hiding this comment

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

This should refer to a published version.

Copy link
Member

Choose a reason for hiding this comment

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

(I just published 0.3.0...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! I was just waiting for a version that worked with the embed bundle.

@jasongrout
Copy link
Member

Some of these are the same changes needed in #1410. Do you have a preference about how to coordinate this with #1410?

@pbugnion
Copy link
Member Author

I was planning on making changes here and rebasing #1410.

@pbugnion pbugnion force-pushed the fix-embed-resolution-issues branch from 38e3d1d to ed50f46 Compare June 20, 2017 05:42
@pbugnion
Copy link
Member Author

This is ready for another look.

The example (jupyter-widgets-htmlmanager/examples/web) is still broken, but I fix it in #1410. I can backport it in a separate PR if we think it's useful to have it fixed as soon as possible.

For testing purposes, this HTML works with this PR and not off master.

<html>
    <head>
        <script src="./embed.js"></script> <!-- This needs the path to the bundled htmlmanager -->
        <script type="application/vnd.jupyter.widget-state+json">

{
    "version_major": 2,
    "version_minor": 0,
    "state": {
        "3d57873d0d8740c099cb3035321f0b8e": {
            "model_name": "SliderStyleModel",
            "model_module": "@jupyter-widgets/controls",
            "model_module_version": "3.0.0",
            "state": {
                "description_width": "",
                "_view_module": "@jupyter-widgets/controls",
                "_model_module": "@jupyter-widgets/controls"
            }
        },
        "cbc3ce11a183401394efe43239c55b26": {
            "model_name": "LayoutModel",
            "model_module": "@jupyter-widgets/base",
            "model_module_version": "3.0.0",
            "state": {}
        },
        "fef054da11f347d4b3414b64ac96eff8": {
            "model_name": "IntSliderModel",
            "model_module": "@jupyter-widgets/controls",
            "model_module_version": "3.0.0",
            "state": {
                "style": "IPY_MODEL_3d57873d0d8740c099cb3035321f0b8e",
                "_view_module": "@jupyter-widgets/controls",
                "layout": "IPY_MODEL_cbc3ce11a183401394efe43239c55b26",
                "_model_module": "@jupyter-widgets/controls"
            }
        }
    }
}

        </script>
    </head>
    <body>
        <h1>hello</h1>
        <script type="application/vnd.jupyter.widget-view+json">
         {
             "model_id": "fef054da11f347d4b3414b64ac96eff8"
         }
        </script>
    </body>
</html>

@jasongrout
Copy link
Member

This looks fine to me. Thanks!

@jasongrout jasongrout merged commit 572cc2c into jupyter-widgets:master Jun 20, 2017
@pbugnion pbugnion deleted the fix-embed-resolution-issues branch June 20, 2017 17:22
@pbugnion
Copy link
Member Author

Awesome, thanks!

@pbugnion pbugnion mentioned this pull request Jun 22, 2017
Closed
@jasongrout jasongrout mentioned this pull request Jun 22, 2017
Merged
3 tasks
@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jasongrout jasongrout jasongrout left review comments

Assignees
No one assigned
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pbugnion @jasongrout