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

encapsulate markdown cells in sphinx directives #46

Closed
kmuehlbauer opened this issue Apr 21, 2016 · 35 comments
Closed

encapsulate markdown cells in sphinx directives #46

kmuehlbauer opened this issue Apr 21, 2016 · 35 comments

Comments

@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Apr 21, 2016

In #45 a special cell metadata switch to encapsulate the whole cell in a sphinx directive in the rendered doc is proposed.

In the rendered doc the cell content should be displayed as eg an admonition (warning, seealso, note,..). At the same time, the look and feel in the notebook should be almost similar.

@kmuehlbauer
Copy link
Contributor Author

Fixed the indentation. This prevented lists to display correctly. Now all markdown should be wrapped correctly into the sphinx-directive.

@kmuehlbauer
Copy link
Contributor Author

Added some sanity check, this will only be wrapped:

  1. if there are more than three lines in the cell and
  2. if the first line (admonition type) corresponds to the second line (correct hyphen count)

Otherwise the cell is processed normally.

@kmuehlbauer
Copy link
Contributor Author

Added nbsphinx_allow_directives option to conf.py.

Now this can be set for the whole doc, notebook-wide by notebook metadata and for the specific cell.

I'm not sure which markdown2rst implementation this should use, currently the nbsphinx-implementation is used.

@kmuehlbauer
Copy link
Contributor Author

ping @mgeier this is ready for review

@mgeier
Copy link
Member

mgeier commented Apr 22, 2016

Thanks for this suggestion!

I thought about a similar thing some time ago, where I wanted to try to add cell metadata like "nbsphinx-note": {} to enable admonitions.
However, I think this is only really nice if this also properly shows in The Notebook (and nbviewer etc.).
Also, to me this sounds like something that should be done within a Markdown cell, like e.g. a blockquote can already be used to set a piece of text apart from the rest.

To move towards this, I asked if there is already a way to do that in jupyter/notebook#1292, but this wasn't received as positively as I hoped. But I'm not giving up so soon ... anyway, before implementing such a thing in Markdown cells, I think it makes sense to switch to CommonMark, which I suggested in jupyter/notebook#1371. This was received slightly more positively, but I daresay it will take some time until this will happen.

So that's blocking that.

Once that is done (and #36 as well), I would like to add a Markdown extension that allows to use admonitions, at least to The Notebook itself, nbconvert and nbsphinx. Having it only in one of them, doesn't sound like a good idea to me.

My current idea for an admonition extension looks like this:

normal text

> !info **Note:** Blah blah blah.
> Blah blah blah.

normal text
  • All happens inside a blockquote, which provides a nice fallback if the extension is not available:

    !info Note: Blah blah blah.
    Blah blah blah.

  • I would allow exactly two admonitions with the case-insensitive "marker" strings !info and !warning. No other types, no translations in other languages.

  • No title is supported. Bold text should be used instead.

  • This could be easily implemented with an AST transform or similar, no complicated manual parsing necessary.

  • This could be mapped to the bootstrap classes alert-info/alert-warning in The Notebook and nbconvert HTML output. In nbsphinx, this could be mapped to the formatting of the note and warning directives but not actually using those, because they would add a title "Note:" and "Warning:" which would probably be language dependent (and not available in The Notebook and nbconvert).

What do you think about all that?

As I said, I would strictly limit this to the two admonitions "info" and "warning", because I think those are enough for all use cases. If I understood correctly, your proposal allows arbitrary Sphinx directives, but you are only talking about admonitions. Are there any other directives you could imagine using with that?

@kmuehlbauer
Copy link
Contributor Author

This took a little while to read and let it sink in.

I thought about a similar thing some time ago, where I wanted to try to add cell metadata like "nbsphinx-note": {} to enable admonitions.
However, I think this is only really nice if this also properly shows in The Notebook (and nbviewer etc.).
Also, to me this sounds like something that should be done within a Markdown cell, like e.g. a blockquote can already be used to set a piece of text apart from the rest.

As you might have seen, this was also the way I thought of first, but it is tedious, if you are working largely with these things. I have also checked the html-stuff first, but found it not really nice working with. And I totally agree to having this done within markdown.

What I gathered from the links (markdown/commonmark) is that a switch to commonmark would allow markdown-extensions, right? Then, you would implement an "admoniton"-extension to use with notebook, nbconvert and nbsphinx. This would definitely put things straight, also with view to the already uses dollar-math-"extension".

If I got this all right, I could work with your proposed admonition-extension. But it's not that clear to me, how this will look in the final html-document (I didn't get the localization stuff).

The reasons to propose the addition in #45 is due to the workflow we use. We had our projects docs written in reST. But with providing more and more code examples we decided to make use of jupyter notebooks. We also wanted to write things only once. Almost everything worked out of the box, except admonitions and citations. After reading all provided information I consider my proposed nbsphinx-addition as a "workaround", which enables us to mimic the behaviour of our reST docs in our notebook workflow. The notebook itself "falls back" to a nice section with the "admonition" type as heading and the cell content as rendered markdown.

If I understood correctly, your proposal allows arbitrary Sphinx directives, but you are only talking about admonitions. Are there any other directives you could imagine using with that?

Yes, I've had only admonitions in mind, and I haven't tested any other directives yet. There won't be much directives left, which can easily incorporated into this. So a renaming to allow_admonitions would make more sense.

Since this "workaround" works for me, I'll use that for the time being until such functionality eventually finds its way into markdown/notebook. Is there anything what I can do, to support this?

@mgeier
Copy link
Member

mgeier commented Apr 25, 2016

a switch to commonmark would allow markdown-extensions, right?

No, we can make extensions in either case.
It just depends on the used library if this is easy or not.
E.g. the LaTeX-math extension that's used currently in The Notebook is quite a hack (because the library doesn't provide an API for syntax extensions).

A switch to CommonMark would be good for other reasons. But since we are already using Markdown extensions now, we have to make sure that the same extensions also work with CommonMark after switching.

... your proposed admonition-extension ... it's not that clear to me, how this will look in the final html-document

I made a proof-of-concept for nbsphinx here: http://nbsphinx.readthedocs.org/en/latest/rst.html#sphinx-directives-for-info-warning-boxes

What do you think of this?

It would probably look something like if you put this into a Markdown cell in the live Notebook:

some text

<div class="alert alert-info">

**Note:** This uses the class `alert-info`

</div>

some text

<div class="alert alert-warning">

**Warning:** This uses the class `alert-warning`

</div>

some text

Since the transition to CommonMark will take a long time, I think this is also the best choice for a work-around.
It's harder to type and uglier to read (in the Markdown source), but at least this already works in the live Notebook and in most other HTML output formats.
I think the fallback of your suggestion doesn't look nice enough in The Notebook, especially since it's impossible to see where the box ends (because there is no box).

As a work-around, I could check for the <div> tags in nbsphinx and convert their content to the new nbinfo and nbwarning directive.

Would this be a reasonable interim solution for you?

Of course, on the long run, I'd much prefer a concise Markdown-like solution.

Yes, I've had only admonitions in mind ...

Here are a few other examples: https://recommonmark.readthedocs.org/en/latest/auto_structify.html#embed-restructuredtext

Is there anything what I can do, to support this?

Yes, you can help convince the Jupyter people that a simple extension for that would be a great thing. This would be a good starting point: jupyter/notebook#1292.
I think that a simple custom solution (like the one I suggested above) is much better than a bulky generic extension syntax as e.g. discussed in http://talk.commonmark.org/t/generic-directives-plugins-syntax/444
You could also review the proposals in https://github.com/jgm/CommonMark/wiki/Proposed-Extensions to see if there is a proposal that would be appropriate for our use case.

@kmuehlbauer
Copy link
Contributor Author

I made a proof-of-concept for nbsphinx here: http://nbsphinx.readthedocs.org/en/latest/rst.html#sphinx-directives-for-info-warning-boxes

This looks really promising. I played a bit with it. If the class, which is set in visit_admonition_html to first only, is set to first admonition-title the box is getting this nice (IMHO) header). This would be the thing we are in need of.

As a work-around, I could check for the <div> tags in nbsphinx and convert their content to the new nbinfo and nbwarning directive.

If I understand correctly, if a markdown cell contains an alert, like this:

<div class="alert alert-info">
**Note:** This uses the class `alert-info`
</div>

nbsphinx converts this to something which looks like the respective sphinx-admonition?

Would this be a reasonable interim solution for you?

Although this means writing this *annoying" html <div> sections, I would adapt to this. The box can be anywhere within a markdown-cell and if the final solution is implemented, the change is just a minor search-and-replace.

@mgeier
Copy link
Member

mgeier commented Apr 26, 2016

If the class [...] is set to first admonition-title the box is getting this nice (IMHO) header. This would be the thing we are in need of.

The problem with that is that, depending on the Sphinx HTML theme, some wanted and some unwanted things could happen:

  • a header box (looks nice, but where does it end? what goes into the "title" and what into the "body"?)
  • a bold and/or larger font (probably unwanted)
  • a symbol before the title (could be nice)
  • a colon after the title (in most cases unwanted)
  • ... probably many more things

Sphinx admonitions have the concept of a "title", which depends on the admonition type and on the selected language. Typically, this is "Note" or "Warning". Sphinx HTML themes are constructed in a way to nicely display those "titles".

I don't think there is a way to bring this concept to The Notebook, especially since there is no concept of a (natural) language used in a notebook.
Therefore, I think the "title" should be simply replaced by some bold text (or not, whatever the author wants), which will work in both The Notebook and in Sphinx.

I agree that the boxes could look a little nicer, especially in the RTD theme. But what about making some local changes to the CSS? I think it should be easy to customize the CSS for admonition.note and admonition.warning.
Note, however, that this will also affect "normal" admonitions created in RST files.

If you find a nice CSS solution, I could use it in the readthedocs-theme branch.

@mgeier
Copy link
Member

mgeier commented Apr 26, 2016

I've made an experimental implementation in #47.
The result can be seen here (warning: those are temporary links!):

The nbviewer page is completely messed up, I guess because <div> content isn't parsed as Markdown ... but that's a different story.
The CSS of The Notebook is still a bit awkward, see jupyter/notebook#1390 and jupyter/notebook#1392.

What do you think?

@kmuehlbauer
Copy link
Contributor Author

@mgeier Thanks! Just had a quick look, seems that could work.

I did some checks with RTD and CSS in the meantime. I'll have a closer look now and report back tomorrow. Hopefully I can come up with a solution which suits bith sides, at least on RTD.

@kmuehlbauer
Copy link
Contributor Author

kmuehlbauer commented Apr 27, 2016

If you use this with the RTD-Theme, the nbsphinx-created admonitions look almost similar to the sphinx' ones. Also sphinx-admonitions wont be changed. Unfortunately I did not get ::before (font-awesome glyphs) working.

div.admonition p.first {
    margin: -12px;
    padding: 6px 12px;
    margin-bottom: 12px;
    color: #fff;
    line-height: 1;
}

div.admonition.warning p.first {
    background: #f0b37e;
}

div.admonition.note p.first {
    background: #6ab0de;
}

@kmuehlbauer
Copy link
Contributor Author

@mgeier Before this turns into an issue: regex.fullmatch doesn't work for python < 3.4.

@mgeier
Copy link
Member

mgeier commented Apr 27, 2016

Oh, I wasn't aware of this! Do you know a more compatible alternative?
If yes, can you please comment on it in #47?

mgeier added a commit that referenced this issue Apr 27, 2016
)

This is an experimental work-around feature!

See also #46.
@kmuehlbauer
Copy link
Contributor Author

@mgeier, #47 and the above CSS work fine on RTD. Shall I launch a pr against the rtd branch with those changes, or have you something more suitable?

@mgeier
Copy link
Member

mgeier commented Apr 27, 2016

Thanks for the CSS, I've already added it to the readthedocs-theme branch, see the result on RTD.

I really like that single-paragraph boxes don't get the header, so you can actually choose if you want the header by using one or multiple paragraphs.

I think it's not possible to get the nice symbols with CSS alone. You'll either need Sass or something to include the other CSS automatically or you need to add the classes fa and fa-exclamation-circle etc. to the HTML. I don't know if this is possible with a Sphinx theme ...?

@kmuehlbauer
Copy link
Contributor Author

OK, cool. To use this RTD I have to checkout the rtd-branch then, right?

I really like that single-paragraph boxes don't get the header, so you can actually choose if you want the header by using one or multiple paragraphs.

Yes, this is what I thought, too. I must admit, that this possibility is much nicer than sphinx-admonitions alone. Hopefully, there will be an markdown-extension soon, to quit fiddling with the html-div's. I'll close #45 and this issue, it's resolved.

@mgeier
Copy link
Member

mgeier commented Apr 27, 2016

OK, cool. To use this RTD I have to checkout the rtd-branch then, right?

No, not at all.

The branch is just meant as documentation. The diff shows what you have to change in your setup!

If you have an idea how to document this more clearly, please tell me.

I could also try to include the additional CSS controlled by a user option ... or I could try to detect if the RTD theme is loaded ... but I'm not sure if that's a good idea ...

If you want to pursue that, feel free to open a new issue!

@kmuehlbauer
Copy link
Contributor Author

Oh I got that totally wrong. I'll add that to our projects-CSS. Thanks again!

@mgeier
Copy link
Member

mgeier commented Apr 27, 2016

When I wrote my last comment above, I wasn't so sure, but the more I think about it, the more I like the idea if those CSS changes could happen automatically.

Do you see any disadvantages in checking for the sphinx_rtd_theme and adding the CSS automatically?

@mgeier
Copy link
Member

mgeier commented Apr 27, 2016

If this is done in nbsphinx, I could probably even add the icons to the HTML!

@kmuehlbauer
Copy link
Contributor Author

Do you see any disadvantages in checking for the sphinx_rtd_theme and adding the CSS automatically?

No, but those might arise later.

If this is done in nbsphinx, I could probably even add the icons to the HTML!

I'd be happy to test this!

@mgeier
Copy link
Member

mgeier commented Apr 27, 2016

I've done it, please test away!

I've also updated all theme branches. There is a little problem with the icons in the "cloud" theme and the "basicstrap" theme is missing some padding, but otherwise, everything looks fine.

@kmuehlbauer
Copy link
Contributor Author

I just had a look at http://nbsphinx.readthedocs.org/. Looks really great!

I'll check that with our docs when I have converted my admonition cells to the alert-divs. Keep you posted.

@kmuehlbauer
Copy link
Contributor Author

OK, I have something. A minor problem is, that markdown (lists) doesn't work in the div wiithin the notebook. This can be solved by using html lists, like this

<div class="alert alert-warning">
**Warning:**<br>

This is an *experimental feature*!
Its usage will probably change in the future or it might be removed completely!

<ul>
  <li>test1</li>
  <li>test2</li>
  <li>test3</li>
</ul>
</div>

This works, the **Warning:** is fitted to the class first, but all the rest is before last and last is empty. I do not know, where to handle this. @mgeier could you have a look please.

JFTR, I found this only, because the empty last introduces a bigger margin at the bottom.

@mgeier
Copy link
Member

mgeier commented Apr 28, 2016

I've seen that problem, but I think there's nothing I can do about it.

I originally also wanted to include a bullet list in a <div>, but since it didn't work, I added it outside: http://nbsphinx.readthedocs.io/en/latest/markdown-cells.html#Info/Warning-Boxes.

This would have to be changed in the Markdown library used in The Notebook (which is called marked). I'm not sure if the behavior is intentional or not. If you want, you can ask about this at jupyter/notebook.
In Sphinx this should work because nbsphinx uses a different Markdown parser (named pandoc).
BTW, nbviewer is broken in a different way, because they use yet another markdown library (I think mistune).

If (or rather "when") The Notebook switches to CommonMark (see jupyter/notebook#1371), this will start working as expected. However, it might take a while until that happens.

Note, however (see the third of the abovementioned bullet points), that you'll have to put in an empty line between the <div> and the start of the content for it to work with CommonMark!
See http://spec.commonmark.org/0.25/#example-152.

JFTR, I found this only, because the empty last introduces a bigger margin at the bottom.

I don't know what you mean by that, can you please give an example?

@mgeier
Copy link
Member

mgeier commented Apr 28, 2016

Also, note that your work-around is not correctly converted to a Sphinx bullet list. It's just keeping the raw HTML, which means that it somewhat works in the HTML output but it looses the bullets in LaTeX output.

@kmuehlbauer
Copy link
Contributor Author

I found some way to overcome this. But, step by step:

I don't know what you mean by that, can you please give an example?

This here in the notebook:

<div class="alert alert-warning">

**Warning:**<br>

This is an *experimental feature*!
Its usage will probably change in the future or it might be removed completely!

<ul>
  <li>test1</li>
  <li>test2</li>
  <li>test3</li>
</ul>

</div>

will render in final doc:

<div class="admonition warning">
<p class="first fa fa-exclamation-circle"><strong>Warning:</strong></p>
<p>This is an <em>experimental feature</em>! Its usage will probably change in the
future or it might be removed completely!</p>
<ul><li><p>test1</p>
</li><li><p>test2</p>
</li><li><p>test3</p>
</li><div class="last"></div></ul></div>

You see, that the last div is at some unwanted position.

But I do it now without lists, but like this in the notebook:

<div class="alert alert-warning">

**Warning:**<br>

This is an *experimental feature*!<br>

- test 1<br>
- test 2<br>
- test 3<br>

Its usage will probably change in the future or it might be removed completely!<br>

- test 4<br>
- test 5<br>
- test 6<br>

</div>

This doesn't look as pleasing in the notebook, but renders fine in the docs:

<div class="admonition warning">
<p class="first fa fa-exclamation-circle"><strong>Warning:</strong></p>
<p>This is an <em>experimental feature</em>!</p>
<ul class="simple">
<li>test 1</li>
<li>test 2</li>
<li>test 3</li>
</ul>
<p>Its usage will probably change in the future or it might be removed
completely!</p>
<ul class="last simple">
<li>test 4</li>
<li>test 5</li>
<li>test 6</li>
</ul>
</div>

@mgeier
Copy link
Member

mgeier commented Apr 28, 2016

The empty <div class="last"></div> is indeed strange ...

Thanks for the work-around. It's ugly as hell, but it indeed works in all 3 cases: The Notebook, Sphinx HTML and Sphinx LaTeX output!

And it looks like it will keep working in CommonMark, too.

@kmuehlbauer
Copy link
Contributor Author

That's good to hear. Ugly as hell, but working. Nevertheless, this should always be used for short alerts, not writing pamphlet's.

One last thing I have, would you mind thinking about allowing all four alerts (.alert-success, .alert-info, .alert-warning or .alert-danger). This would be very nice, indeed!

@mgeier
Copy link
Member

mgeier commented Apr 28, 2016

I've already thought about this and I currently think that "info" and "warning" are enough (see #46 (comment)).

Can you name a few examples where you would want to use "success" or "danger"?

@kmuehlbauer
Copy link
Contributor Author

I would use:

  • "info", to give the user additional information.
  • "warning", to warn the user about some glitches with software
  • "success", to show the user some positive things, like new features
  • "danger", to alert the user about a possible danger, which might destroy soft- or hardware

So, "success" is the positive form of "info", it should contrast to the more normal "info". "warning" should be used to alert the user about something which goes beyond a normal "info". "danger" is for the real hard cases and should be in contrast to "warning". I hope you can follow my thoughts.

@mgeier
Copy link
Member

mgeier commented Apr 28, 2016

Sure, I can follow.

But this is all very theoretical. I'd still like to see more concrete examples.
The thing with the new features may sound intriguing, but I don't really see anything related to a "success" there.

I think the bootstrap alerts were made for a very different use case, namely to show responses of, well, responsive web apps. IMHO, something like a static notebook text doesn't need a "success".

And "warning" and "danger" are basically synonymous anyway, so why bother?
Would there be any "danger" that's not worth warning about?
Would you issue a "warning" about something that's not at all dangerous?
Can you imagine a case where you have two boxes side by side and one is clearly a "warning" and the other clearly a "danger"? What would you loose if you didn't have the distinction?

And do you really need more than 2 colors to express your feelings?

Having said all that, there is a much more pragmatic side to this.

I checked all the Sphinx themes I've listed in the docs for how they display the different available Sphinx admonitions (which are a lot: attention, caution, danger, error, hint, important, note, tip, warning and admonition).

I found out that most themes use 2 different colors and only very few use 3 or 4 (I don't remember if any of them even had 4). Interestingly, the assignment of different admonition types to those 2 colors was quite different, there was only one common thing: All themes had "note" and "warning" in different colors.
I probably should have read the docs before, because they already state:

Most themes style only “note” and “warning” specially.

So that's that. Even if I would support more than 2 admonition types, there wouldn't be more than two different colors in most Sphinx themes after all. So why bother?

@kmuehlbauer
Copy link
Contributor Author

kmuehlbauer commented Apr 28, 2016

Thanks for your patience. I'm not sure if I really need more than the two in the end. Your arguments make sense, but mine too (at least to me). I'm happy to have a more neat solution for my admonition-problem now, so I'll stop arguing, and just be happy.

@mgeier
Copy link
Member

mgeier commented Apr 28, 2016

I like arguing and I'm convinced that it can make the software better, so please don't hesitate to continue arguing. I'm very much interested in the opinions of other people.

My decision about only 2 admonition types is not final. But if you want more, you'll have to make a suggestion how to handle the fact that most Sphinx themes will only show 2 colors anyway.
And you'll have to present compelling examples for the additional admonition types.

We can also continue this discussion at any point later in time. Adding things later is typically no problem, removing things often is ...

mgeier added a commit that referenced this issue Feb 3, 2019
)

This is an experimental work-around feature!

See also #46.
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

No branches or pull requests

2 participants