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
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr 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.

@jlstevens
Copy link
Contributor

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

@philippjfr
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@philippjfr philippjfr Apr 17, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

embed_js = """
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@philippjfr philippjfr added the type: bug Something isn't correct or isn't working label Apr 17, 2018
@jlstevens
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
@philippjfr philippjfr deleted the html_export branch July 4, 2018 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: notebook type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants