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

Make nbconvert html full output like notebook's html. #5232

Merged
merged 22 commits into from Mar 7, 2014

Conversation

jdfreder
Copy link
Member

Basically makes the DOMs of both map one to one. This doesn't mean string notebook html == string nbconvert output.

I used a custom in-house HTML diffing rig to diff, see here https://gist.github.com/jdfreder/9241376 (mentioned in the README.md inside nbconvert/tests).

related to #5084
closes #5224

@@ -40,8 +40,10 @@ In [{{ cell.prompt_number }}]:
{% endblock output_prompt %}

{% block input %}
<div class="input_area box-flex1">
<div class="inner_cell">
<div class="input_area">
Copy link
Member

Choose a reason for hiding this comment

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

No box-flex1 on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't in the normal notebook's html.

@jdfreder
Copy link
Member Author

jdfreder commented Mar 4, 2014

@ellisonbg this is ready for review.

@jdfreder jdfreder added this to the 2.0 milestone Mar 4, 2014
@@ -59,8 +61,7 @@ In&nbsp;[{{ cell.prompt_number }}]:
{% endblock output %}

{% block markdowncell scoped %}
<div class="cell border-box-sizing text_cell">
<div class="input">
<div class="cell border-box-sizing text_cell rendered unselected">
Copy link
Member

Choose a reason for hiding this comment

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

I think adding unselected is probaby too much

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree - just like we don't include the mode classes. The selected
state is nor relevant in static output.

On Wed, Mar 5, 2014 at 11:24 AM, Min RK notifications@github.com wrote:

In IPython/nbconvert/templates/html/basic.tpl:

@@ -59,8 +61,7 @@ In [{{ cell.prompt_number }}]:
{% endblock output %}

{% block markdowncell scoped %}
-


-

+

I think adding unselected is probaby too much

Reply to this email directly or view it on GitHubhttps://github.com//pull/5232/files#r10314022
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member

Two questions:

  • What should the outer element be in the basic template. Right now it is just a bunch of cell divs with no containing div.
  • What should the outer elements be in the full template. This PR adds all of the one we use in the live notebook, which is probably overkill, but which should we keep?

<div style="display: block;" id="site" class="border-box-sizing">
<div id="ipython-main-app" class="border-box-sizing">
<div id="notebook_panel" class="border-box-sizing" style="-webkit-box-shadow: none; -moz-box-shadow: none; box-shadow: none;">
<div tabindex="-1" id="notebook" class="border-box-sizing" style="overflow: visible; border-top: none;">
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, there shouldn't be any style attributes set anywhere in the template. If you leave these out, is the notebook invisible?

Copy link
Member Author

Choose a reason for hiding this comment

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

What I can do is move them into the <style> tag above where the rest of the CSS is, that's where they belong.

As far is what each one does:
"overflow: visible;" removes the extra vertical scrollbar. Without this the browser's vertical scroll bar renders with very little scrollable space and another vertical scroll bar renders inside the document, around the notebook contents, which handles the scrolling of the document.
"border-top: none;" removes a small 1px grey border from the top of the notebook
shadow stuff- removes a very faint shadow that surrounds the whole document

@jdfreder
Copy link
Member Author

jdfreder commented Mar 6, 2014

So this is ready for some more review: Upon auditing our output_* CSS classes, I found that we did not have a matching output_pyout for the already existing output_pyerr. I went ahead and added it to both nbconvert and the notebook. This required me to add extra_class in both the nbconvert template and outputarea.js since pyout uses the existing machinery for displaying other types. I borrowed extra_class idea from the append_text function in outputarea.js.

@@ -498,11 +498,11 @@ var IPython = (function (IPython) {
} else {
// don't display if we don't know how to sanitize it
console.log("Ignoring untrusted " + type + " output.");
continue;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

this indentation is now incorrect, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wonder how that got in there, maybe my Sublime is using tab characters or something, I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah it was a rebase problem.

@jdfreder
Copy link
Member Author

jdfreder commented Mar 6, 2014

Can't we just add the class at the outer level, rather than passing it as an arg (e.g. remove extra_class from append_text, rather than adding it to all)?

That's a good idea, I'll take a look but I think you may be right.

@jdfreder
Copy link
Member Author

jdfreder commented Mar 6, 2014

@minrk that looks much better, thanks for the suggestion.

@@ -56,7 +56,7 @@ def __call__(self, source, language=None, metadata=None):
if not language:
language=self.default_language

return _pygments_highlight(source, HtmlFormatter(), language, metadata)
return _pygments_highlight(source if len(source) > 0 else ' ', HtmlFormatter(), language, metadata)
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason for this change?

Copy link
Member

Choose a reason for hiding this comment

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

Empty code cells where collapsing vertically (to something like 5 px). This
gives them the right height when empty.

On Thu, Mar 6, 2014 at 3:50 PM, Min RK notifications@github.com wrote:

In IPython/nbconvert/filters/highlight.py:

@@ -56,7 +56,7 @@ def call(self, source, language=None, metadata=None):
if not language:
language=self.default_language

  •    return _pygments_highlight(source, HtmlFormatter(), language, metadata)
    
  •    return _pygments_highlight(source if len(source) > 0 else ' ', HtmlFormatter(), language, metadata)
    

what is the reason for this change?

Reply to this email directly or view it on GitHubhttps://github.com//pull/5232/files#r10367691
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Copy link
Member Author

Choose a reason for hiding this comment

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

collapsing because codemirror doesn't exist

@ellisonbg
Copy link
Member

When this is merge, we need to submit a PR to nbviewer to remove the lines that style the input area, highlight, pre stuff here:

https://github.com/ipython/nbviewer/blob/master/nbviewer/templates/notebook.html#L60

This styling is now done in the notebook CSS itself.

Lots of CSS tweaks to get nbconvert output looking right.
minrk added a commit that referenced this pull request Mar 7, 2014
Make nbconvert html full output like notebook's html.
@minrk minrk merged commit b668b92 into ipython:master Mar 7, 2014
@jdfreder jdfreder deleted the nbc_incono branch March 10, 2014 18:43
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Make nbconvert html full output like notebook's html.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit nbconvert HTML output
3 participants