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

MimeRenderer Extensions #2488

Merged
merged 35 commits into from Jun 22, 2017
Merged

MimeRenderer Extensions #2488

merged 35 commits into from Jun 22, 2017

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 20, 2017

Continuation of #2482. Fixes #2354.

  • Add a declarative interface for mime renderer extensions
  • Add a registration method for mime renderer extensions
  • Add a widget and widget factory for mime renderer extensions
  • Use the registered mime renderers and add them to the rendermime and create
    widget factories if applicable
  • Thread the handling of mime extensions through to the build system
  • Rebase after #2480 and finish MimeRenderer implementation
  • Break out the rendermime-interfaces package
  • Add state restoration for mime renderer opened files
  • Finish tests
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 20, 2017

TODO: break out docregistry handler into a separate plugin

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jun 21, 2017

I am testing this locally by updating Altair to use this MIME based renderer. So far working great. I will have some minor fixes to the vega package in the PR that I will submit as a PR to yours. I will then follow up with code review on the core code. Awesome to see it working though!

screen shot 2017-06-20 at 8 41 59 pm

@blink1073 blink1073 changed the title [WIP] MimeRenderer Extensions MimeRenderer Extensions Jun 21, 2017
Copy link
Contributor

@ellisonbg ellisonbg left a comment

OK I have done a pretty detailed review and have looked at every file here, by package, starting with the interfaces, then rendermime/doc stuff and outwards. It looks fantastic, what a tour de force! The main thing is to make sure we figure out when a renderer expect JSON versus strings. It is important to make sure we are not encoding/decoding strings to JSON and back more than needed as some of the mime bundles can be large. I can have another look first thing in the morning.



/**
* A namespace for IRenderMime associated namespaces.
Copy link
Contributor

@ellisonbg ellisonbg Jun 22, 2017

IRenderMime associated interfaces.

@@ -0,0 +1,418 @@
// Copyright (c) Jupyter Development Team.
Copy link
Contributor

@ellisonbg ellisonbg Jun 22, 2017

I have reviewed this file. Really nice to have them all together, and I feel good about having this be the one thing that external renderers have to import. By now the interfaces are clean and reasonably stable. Very nice!

* Add the link handler to the node.
*/
handleLink(node: HTMLElement, url: string): void;
function registerExtensionModule(mod: IRenderMime.IExtensionModule): void {
Copy link
Contributor

@ellisonbg ellisonbg Jun 22, 2017

Looks good, really clean.

let model = context.model;
let layout = this.layout as PanelLayout;
let data: JSONObject = {};
data[this._mimeType] = model.toString();
Copy link
Contributor

@ellisonbg ellisonbg Jun 22, 2017

This may be the culprit in the Vega extension not working. I think it expects JSON data, rather than a string. We ran into this before, but I don't remember how we resolved it. We probably need to a figure out a way have a clear way of handling MIME types that have JSON or strings for data.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 22, 2017

The file data from the server is always text unless it is a notebook. I added a dataType option that is threaded through to the mime renderer, and vega files are now rendering properly.
I also added state restoration for mime renderers.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jun 22, 2017

Looks great now, merging!

@ellisonbg ellisonbg merged commit 1e7bd07 into jupyterlab:master Jun 22, 2017
2 checks passed
@blink1073 blink1073 mentioned this pull request Jul 7, 2017
@blink1073 blink1073 deleted the pr/2482 branch Jul 10, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
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.

2 participants