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

Export output widget in embed bundle #1410

Merged
merged 40 commits into from Jun 24, 2017

Conversation

@pbugnion
Copy link
Member

@pbugnion pbugnion commented Jun 5, 2017

Prior to this PR, the output widget could not be embedded. Addresses issue #986 .

This is a rewrite of PR #1380, which had become out of date after the htmlmanager was moved to a stand-alone package.

I think this still needs:

  • Unittest widget (e.g. with a mock manager)
  • migrate rendermime to a singleton or to the HTMLManager to avoid re-instantiating it for every output area
  • Expose ability to define new renderers (and override the default ones).
  • Custom CSS for the output area (at the moment, it doesn't look great)
@pbugnion pbugnion force-pushed the output-area-embed-widget branch 3 times, most recently from ffda808 to 049d035 Jun 11, 2017
@pbugnion
Copy link
Member Author

@pbugnion pbugnion commented Jun 12, 2017

@jasongrout I'm about to start work on letting users override renderers in the HTML Manager. How were you thinking users would hook into this?

One way would be to define a top-level method in embed-webpack (e.g. bound to window) called configureWidgets that defines top-level options. The options would be stored in some form of global state.

Alternatively, we could just expose the options as an extra argument to renderInlineWidgets. We are then just catering for users who use the htmlmanager as a library and who instantiate the widgets themselves. If we go down that route, I'll add another example demonstrating how to do this.

I think I'm mildly in favour of the second option, since it exposes less magic.

Loading

@jasongrout
Copy link
Member

@jasongrout jasongrout commented Jun 12, 2017

IIRC, a top-level method won't work since you might have multiple instances of the widget manager on a single page - I think this may happen in some of the documentation scenarios.

Since your rendermime instance is exposed as a public attribute on the manager (i.e., the manager's rendermime is what is passed to the output widget), I think that may be enough - people can access the rendermime directly and add other renderers as needed.

Loading

@@ -45,11 +54,11 @@ 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

@jasongrout jasongrout Jun 17, 2017

Choose a reason for hiding this comment

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

The @jupyter-widgets/base base classes (like DOMWidgetView, WidgetModel, etc.) shouldn't be exported in @jupyter-widgets/controls - I think you'll need two checks here, one for each of your coreWidgetModules. See

if (moduleName === "@jupyter-widgets/controls") {

Loading

new RenderMime({
items: [
{ mimeType: WIDGET_MIMETYPE, renderer: new WidgetRenderer(this) },
... RenderMime.getDefaultItems()
Copy link
Member

@jasongrout jasongrout Jun 17, 2017

Choose a reason for hiding this comment

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

please delete the space after ...

Loading

@pbugnion pbugnion mentioned this pull request Jun 19, 2017
@jasongrout jasongrout added this to the 7.0 milestone Jun 19, 2017
@jasongrout
Copy link
Member

@jasongrout jasongrout commented Jun 22, 2017

Also, I changed a bit how testing is done and how the repos are built. There's only a few conflicts, but perhaps resolve those and rebasing or merging master in will help isolate the failures.

Loading

@jasongrout
Copy link
Member

@jasongrout jasongrout commented Jun 23, 2017

@pbugnion - I was looking into updating this. Are you working on it too?

Loading

@pbugnion
Copy link
Member Author

@pbugnion pbugnion commented Jun 23, 2017

@jasongrout I definitely won't be working on this today, and probably won't be able to over the weekend, so go ahead! Thanks.

Loading

@jasongrout jasongrout force-pushed the output-area-embed-widget branch from c4f34ce to e2afc49 Jun 23, 2017
@jasongrout
Copy link
Member

@jasongrout jasongrout commented Jun 24, 2017

@pbugnion - I think it should work, but I haven't tested it to see if it actually does yet.

Loading

@jasongrout jasongrout force-pushed the output-area-embed-widget branch from 1046670 to cff5f5a Jun 24, 2017
@jasongrout
Copy link
Member

@jasongrout jasongrout commented Jun 24, 2017

I just tested by hand (modified the example with some state of an output widget), and the text and an embedded widget worked fine. I'll clean up one last thing with the model_id rename and merge when tests pass.

Loading

@jasongrout
Copy link
Member

@jasongrout jasongrout commented Jun 24, 2017

Actually, let's make the model_id rename a different PR, since it's really a different issue. It came up here because the widgetsnbextension output widget is broken without it.

Loading

@jasongrout jasongrout force-pushed the output-area-embed-widget branch from cff5f5a to b3e0c81 Jun 24, 2017
@jasongrout jasongrout merged commit 9de6cee into jupyter-widgets:master Jun 24, 2017
1 check passed
Loading
@jasongrout jasongrout changed the title [WIP] Export output widget in embed bundle Export output widget in embed bundle Jun 24, 2017
@pbugnion
Copy link
Member Author

@pbugnion pbugnion commented Jun 24, 2017

Ah this is great -- thanks for your help!

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants