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

Add support for HTML export after moving to MIME bundle #2574

Merged
merged 5 commits into from Apr 17, 2018

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

philippjfr commented Apr 17, 2018

When we switched to rendering our plots using the _repr_mimebundle we lost all support for exporting bokeh plots and holoviews widgets to standalone HTML files. This is because nbconvert will only render a single MIME type, whichever is highest in its priority ordering. Since application/javascript has the highest priority only that gets rendered and doesn't end up finding the HTML tags it's meant to render into.

The only approach I could possible see how to make it work is to dynamically create the HTML DOM nodes that the JS renders into. So this PR adds a bit of JS code to each plot that includes application/javascript, which looks for an existing DOM element with the matching element_id and if that doesn't exist it dynamically creates it on its own parent node. This means that in the notebook nothing changes but in a static HTML export the DOM nodes are appropriately created.

I have to say I really dislike this approach but it is absolutely the only way I see to make this possible, since I can only publish a single MIME type in the nbconvert output and application/javascript will always take precedence.

@philippjfr philippjfr added the notebook label Apr 17, 2018

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Apr 17, 2018

Isn't this arguably a bug in nbconvert? Shouldn't the output of _repr_mimebundle have the highest precedence?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Apr 17, 2018

Isn't this arguably a bug in nbconvert?

No.

Shouldn't the output of _repr_mimebundle have the highest precedence?

No, the output of _repr_mimebundle contains a bunch of things and only the thing with the highest precedence that is supported by the output type (in this case HTML) will make it into the converted output. According to the nbconvert docs each MIME type should be an alternative but standalone representation of the plot. So what this PR does is make the highest priority item (namely application/javascript) be able to render itself as a standalone thing.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Apr 17, 2018

According to the nbconvert docs each MIME type should be an alternative but standalone representation of the plot.

That doesn't work when you have to separate HTML from JS which you now have to do for Jupyterlab. I agree this is by 'design' but that design now looks broken to me.

element_id = plot_id
html = "<div id='%s' style='display: table; margin: 0 auto;'>%s</div>" % (element_id, html)
if not os.environ.get('HV_DOC_HTML', False) and js is not None:
js = embed_js.format(element_id=element_id, plot_id=plot_id, html=html) + js

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2018

Contributor

Why define element_id if you can just replace this with plot.id (or plot_id)?

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2018

Contributor

Nevermind, I see it now. Though isn't it plot.id in both branches?

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2018

Author Contributor

Bit confusing but in the widget case they aren't identical, in that case plot variable is actually the widget and the two are different. I might just call it widget_id instead of element_id and then default it to None in the non-widget case.

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2018

Contributor

Ok. If it can be made clearer, then great!

@@ -69,6 +69,17 @@
</html>
"""

embed_js = """

This comment has been minimized.

@jlstevens

jlstevens Apr 17, 2018

Contributor

I suggest leaving a comment pointing to this PR where we can explain why such a nasty thing exists.

This comment has been minimized.

@philippjfr

philippjfr Apr 17, 2018

Author Contributor

Agreed, don't want anyone blaming us for this.

Philipp Rudiger Philipp Rudiger

@philippjfr philippjfr force-pushed the html_export branch from f97769f to 24b5b13 Apr 17, 2018

@philippjfr philippjfr added the bug label Apr 17, 2018

@philippjfr philippjfr force-pushed the html_export branch from 24b5b13 to abb167e Apr 17, 2018

Philipp Rudiger Philipp Rudiger
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Apr 17, 2018

Looks as good as it is going to get and the tests have finally gone green.

For anyone directed to this PR from the JS comment, this is a very ugly hack that we did not want to include in our code. Unfortunately, the alternatives were even less palatable 1) prematurely drop support for classic Jupyter notebook in favor of JupyterLab 2) send all the data to the notebook twice.

Given that we found those alternatives unacceptable, we opted for this approach which should only ever execute in the context of HTML exported using nbconvert.

@jlstevens jlstevens merged commit 285533c into master Apr 17, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 82.702%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the html_export branch Jul 4, 2018

@Chilipp Chilipp referenced this pull request Sep 16, 2018

Merged

new SOSE use case #386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.