NBsphinx spacing after code cell

[adamgayoso]

I've noticed a few peculiarities with nbsphinx integration.

First, the code cell tends to be of lesser width than the text, and if a text cell succeeds a code cell, the spacing between them is quite tight.

Edit: This can also be seen using a version of the nbsphinx docs.

[jorisvandenbossche]

@adamgayoso Thanks for the report! That can indeed be improved.

Quickly looking into it, the first issue of the padding on the left/right side of the code cell seems to be due to the container class used by the divs of nbsphinx, and we have this piece of css:

.container {
 width:100%;
 padding-right:15px;
 padding-left:15px;
 margin-right:auto;
 margin-left:auto
}

which I think is inherited by bootstrap. So not directly sure what to do about it. Maybe we could disable this css only if the div also has a docutils class (the div for the nbsphinx code cell uses <div class="nbinput nblast docutils container">.

For the vertical padding issue (the next text paragraph being too close to the nbsphinx cells): I was comparing with the version using readthedocs theme (https://nbsphinx.readthedocs.io/en/rtd-theme/code-cells.html), and for that theme, it seems that nbsphinx has some custom css to have it look better there: https://github.com/spatialaudio/nbsphinx/blob/31df9169555b31d663972507449a45937da5fbce/src/nbsphinx.py#L685-L707

So either we could ask nbsphinx to do something similar for this theme, or we could include that ourselves in the css of this theme to ensure nbsphinx looks good.
I think that the reason that it looks good out of the box for most other themes, is because <p> text paragraphs have some top/bottom margin themselves (while this theme and rtd theme only seems to have a bottom margin). So another option could also be to explore adding a top margin to <p> elements.

[damianavila]

OK, I have researched this one a little bit :wink:

which I think is inherited by bootstrap

Effectively, the .container class comes from Bootstrap.

Maybe we could disable this css only if the div also has a docutils class (the div for the nbsphinx code cell uses

.

In general, adding special cases in a parent theme like this one to support other projects stuff should not be the answer, IMHO. Additionally, I think modifying a general bootstrap class like this one should be an inclusive and hugely tested proposal because of the potential impact of the change (high breaking power).

it seems that nbsphinx has some custom css to have it look better

Since nbsphinx has already a way to deal with different themes, I think we should go in that direction and adjust the CSS upstream: https://github.com/spatialaudio/nbsphinx/blob/master/src/nbsphinx.py#L686 (propose a CSS_STRING_PYDATASPHINXTHEME). Some incantation of the following rules make things to look nice, IMHO:

  div.nbinput.container, div.nboutput.container {
    padding-left: unset;
    padding-right: unset;
  }
  div.nbinput.container div.input_area, div.nboutput.container div.output_area {
    padding-left: unset;
    padding-right: unset;
  }

I think that the reason that it looks good out of the box for most other themes, is because

text paragraphs have some top/bottom margin themselves (while this theme and rtd theme only seems to have a bottom margin). So another option could also be to explore adding a top margin to

elements.

In this case, I think even when modifying <p> elements is potentially huge in terms of the impact, I do think the risk is far lower than the previous case and we are, in fact, already being opinionated about this piece: https://github.com/pydata/pydata-sphinx-theme/blob/master/src/scss/_base.scss#L16. Other themes use margin at the top and the bottom, ie. Furo:

          _typography.sass
            // Paragraph
            p
              margin-top: 0.75rem
              margin-bottom: 0.75rem

So, I guess it would be a change we can do in our CSS (maybe using Furo defaults) instead of pushing the request upstream.

@jorisvandenbossche @choldgraf thoughts?

[choldgraf]

@damianavila to summarize both your suggestions to make sure I understand them:

1. Suggest an upstream improvement in nbsphinx to special-case the pydata theme
2. Add a top margin to our p elements since that will in general encourage better behavior as a default

And you're proposing to do both, is that right?

If so, then yes those both sound reasonable to me. Maybe the easiest thing is to start with 2 and open an issue in nbsphinx with your proposed CSS to see what the dev there thinks?

[damianavila]

And you're proposing to do both, is that right?

Yep, sorry if I was not clear enough with my proposal.

Maybe the easiest thing is to start with 2 and open an issue in nbsphinx with your proposed CSS to see what the dev there thinks?

Sounds like a plan to me 😉 .

[jorisvandenbossche]

Sounds good!