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

Minor nbsphinx-related tuning #1266

Merged
merged 6 commits into from
Apr 5, 2016
Merged

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Mar 26, 2016

I wanted to introduce nbsphinx to the Notebook docs but then I saw that @willingc beat me to it: #1255

That's great, thanks!

Here I have just a few minor adjustments (hopefully "improvements") ...

@@ -0,0 +1,9 @@
/* 24px margin from readthedocs theme */
div.nblast {
margin-bottom: 19px !important; /* padding has already 5px */
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to do this without !important? that's always something we should avoid if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I couldn't find one.

But I think in this case it doesn't matter, because there will be no further CSS "derived" from this.

Anyway, if somebody knows a way, I'm happy to change this!

Copy link
Member

Choose a reason for hiding this comment

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

@jdfreder Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

where is .nblast created ? Can't you use somehting like :last-child ? Or an example page that use it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

where is .nblast created ?

Ditto, I just checked out the PR, cleaned and built the docs, and opened one of the nbsphinx docs (example notebooks) and I didn't see it on the page. document.querySelector('.nblast')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.nblast is used wherever there are code cells, e.g. in https://jupyter-notebook.readthedocs.org/en/latest/examples/Notebook/Running%20Code.html

I didn't want to create a separate reST directive as container for a "code cell" (i.e. the combination of an nbinput directive and 0..N nboutput directives). Currently, the nbsphinx directives for input/output cells can be used on the top level, see http://nbsphinx.readthedocs.org/en/latest/rst.html#sphinx-directives-for-jupyter-notebook-cells.
Since I don't have a reST container, I also don't know how to get the parts of a "code cell" into a CSS container like the code_cell class used by the Notebook/nbviewer. If you know a (reasonably simple) way to do that, please tell me!
As a work-around, I introduced the :no-output: and :more-to-come: flags for the nbinput/nboutput directives, which translate to the nblast CSS class. This class is used in the last group of a "code cell", i.e. in an input cell if no output is available or in an output cell if it's the last of a group of outputs.

Since (currently) there is no CSS container class for a "code cell" in nbsphinx, I cannot use :last-child. I also wouldn't have to if there were, because then I could simply use the margin settings of the container.

But probably I can do this with some other CSS magic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option would be to fix the RTD theme itself, but I can't do that since I have no clue about SASS.

Copy link
Member

Choose a reason for hiding this comment

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

so

div.nbinput.nblast {
    margin-bottom: 19px;  /* padding has already 5px */
}

See to work for me, and the second !important seem to have no effect to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Carreau! I added your suggestion in 9c56f45.

@@ -118,7 +118,7 @@

# List of patterns, relative to source directory, that match files and
# directories to ignore when looking for source files.
exclude_patterns = ['_build']
Copy link
Member

Choose a reason for hiding this comment

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

do we want to keep _build in the list of ignored patterns ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When building the docs with make html (as described in the docs), the built files end up in docs/build/ and not in docs/source/_build/, so I don't think it's necessary to have latter in the exclude_patterns.

But if you want it there anyway, just say the word, and I'll add it back!

Copy link
Member

Choose a reason for hiding this comment

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

No problem, thanks for the precision.

@Carreau
Copy link
Member

Carreau commented Apr 4, 2016

1 comment, +1 otherwise

@Carreau Carreau merged commit cbd9708 into jupyter:master Apr 5, 2016
@Carreau
Copy link
Member

Carreau commented Apr 5, 2016

Ok, merged, let's see if people complain.

willingc added a commit that referenced this pull request Apr 6, 2016
Remove custom.css added in #1266
Closes #1305
@mgeier mgeier deleted the nbsphinx-tuning branch April 12, 2016 08:24
@minrk minrk modified the milestone: 5.0 Apr 15, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants