spatialaudio / nbsphinx

:ledger: Sphinx source parser for Jupyter notebooks
https://nbsphinx.readthedocs.io/
MIT License
449 stars 128 forks source link

use inline literal instead of a `literal_block` in execution counts #732

Closed 2bndy5 closed 1 year ago

2bndy5 commented 1 year ago

https://github.com/spatialaudio/nbsphinx/blob/f1d3f728f979d77d0e1d356b3d51a2336a94f6dc/src/nbsphinx/__init__.py#L704-L705

This seems unnecessary as an inline literal node (docutils.nodes.literal) will suffice. I raise this issue because our sphinx-immaterial theme's inherited JS (from mkdocs-material theme) injects a copy-to-cliboard button for literal blocks. For reference, see https://github.com/jbms/sphinx-immaterial/issues/218#issuecomment-1529111519

The CSS may have to be adjusted to not show the background color of the inline literal: https://github.com/spatialaudio/nbsphinx/blob/f1d3f728f979d77d0e1d356b3d51a2336a94f6dc/src/nbsphinx/_static/nbsphinx-code-cells.css_t#L19-L22

changed to:

div.nbinput.container div.prompt *,
div.nbinput.container code.prompt,
div.nboutput.container div.prompt * {
    background: none;
}
mgeier commented 1 year ago

Thanks for reporting this!

Would you like to make a PR?

FYI, there is special CSS for the sphinx_copybutton extension:

https://github.com/spatialaudio/nbsphinx/blob/f1d3f728f979d77d0e1d356b3d51a2336a94f6dc/src/nbsphinx/_static/nbsphinx-code-cells.css_t#L207-L210

But it would of course be great if there is a more general solution that would work with all types of copy buttons.

However, we could also add another CSS rule for the sphinx-immaterial theme.

2bndy5 commented 1 year ago

Would you like to make a PR?

Sure.

However, we could also add another CSS rule for the sphinx-immaterial theme.

AFAIK, this issue is the only compatibility problem. There were some problems that seemed to be resolved with v0.9.0 refactor 👍🏼 . I'm glad to hear you're open to the idea though.


Regarding the sphinx-copybutton ext, our JS uses the same npm pkg, but the mounting algorithm may be different. I think this change should be more optimal than hiding a copy button that was injected via JS.

Full disclosure, I haven't tested the suggested changes on other themes (nor sphinx-copybutton ext), so I'll have to do some prelimary testing with your docs before submitting the PR.


Comment moved to new thread #734 A separate issue I just noticed with the furo theme example is the background color for stderr output isn't compatible with the dark scheme: ![image](https://user-images.githubusercontent.com/14963867/235434593-e81c3f6b-30ac-496d-a945-cca5c54d8e44.png) but using a transparency with slightly increased saturation should remedy that in a consistent way for light or dark schemes: ```css /* nbsphinx-code-cells.css | https://nbsphinx.readthedocs.io/en/furo-theme/_static/nbsphinx-code-cells.css */ div.nboutput.container div.output_area.stderr { /* background: #fdd; */ background: #e8808075; } ``` ![image](https://user-images.githubusercontent.com/14963867/235434508-b4a9b431-0962-4415-9b0d-17b938ea902f.png)
2bndy5 commented 1 year ago

I'm inclined to remove the mentioned sphinx-copybutton rule in the CSS template because it doesn't seem necessary with this suggested change. I did so locally and tested the docs with the ext enabled, and it renders fine; the copybutton is only injected in the input code blocks. I find this acceptable because input code blocks content aren't always specific to Jupyter notebooks.

I'm still going through the CSS template to find necessary updates to execution counts (eg. colors, margins/padding, alignment, etc.). I should have a PR ready by end-of-day.