spatialaudio / nbsphinx

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

hide copy-button in sphinx-immaterial theme #737

Closed 2bndy5 closed 1 year ago

2bndy5 commented 1 year ago

resolves #732

Branch reset to use the original approach in which the JS is not bypassed and the resulting copy-button is hidden in nbsphinx CSS.

Despite this approach being less performant with ClipboardJS usage, this approach seems to be the only feasible way to achieve the desired result; see https://github.com/spatialaudio/nbsphinx/pull/737#issuecomment-1535492952 for a detailed analysis.

mgeier commented 1 year ago

Thanks for this PR.

It seems a bit brittle to me, though.

In some themes the alignment is off, sometimes the prompt color is missing, and sometimes the font size is different between prompts and code cells.

I think it is more reliable if the prompt uses the same HTML structure as the code cells.

Wouldn't it be possible to hide to copybutton with a CSS rule similar to how it's done for sphinx_copybutton?

2bndy5 commented 1 year ago

It is possible to hide it, but it isn't performant because the JS still goes through and adds the copy-button for each prompt on page load.

I didn't notice any color discrepancies. I did see varying alignments though.

2bndy5 commented 1 year ago

Maybe there's a way to still use literal_block with attributes that make the JS injection skip the exec counts...

The sphinx-copybutton ext uses the following selector as a default: https://github.com/executablebooks/sphinx-copybutton/blob/4a8050dea36536b005d3e757b1ac09886174f634/sphinx_copybutton/__init__.py#L82 This would mean that the existing exception to hide the copy button would still be required if the doc author changes the selector in conf.py. But removing the highlight class should prevent the default selector from finding the exec counts. However, the highlight class seems to be added by pygments in visit_literal_block().

Unfortunately, the sphinx-immaterial theme's JS injection isn't as selective: https://github.com/jbms/sphinx-immaterial/blob/6d4b9eb02a6691a719654f1fdb7783dd98461df6/src/assets/javascripts/components/content/_/index.ts#L97

I'll reset this branch and push a new CSS exception for the sphinx-immaterial theme's copy-button.

2bndy5 commented 1 year ago

Not exactly sure why the RTD build failed. It just says:

ERROR: Can not execute `setup.py` since setuptools is not available in the build environment

But, I see it specified in https://github.com/spatialaudio/nbsphinx/blob/a619a5091c29815c1605166d6adca26700fae6e9/pyproject.toml#L2

Maybe there's a change in the v9.12.0 release (2 days ago) of readthedocs.org

mgeier commented 1 year ago

Not exactly sure why the RTD build failed.

The problem should be fixed in the master branch, can you please rebase?

mgeier commented 1 year ago

When I try to build the docs with the immaterial theme, I get this error:

Exception occurred:
  File "/home/mg/git/sphinx/sphinx/domains/python.py", line 575, in handle_signature
    signode += _parse_arglist(arglist, self.env, multi_line_parameter_list)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: _monkey_patch_python_parse_arglist.<locals>.parse_arglist() takes from 1 to 2 positional arguments but 3 were given

Do you know this error? What can I do to avoid it?

BTW, building on RTD timed out, currently it's running here: https://readthedocs.org/projects/nbsphinx/builds/20768938/

Do I have to do anything special on RTD?

2bndy5 commented 1 year ago

I haven't come across that error before. There's a lot of monkey-patches to Sphinx domains in the theme.

I don't do anything special for RTD in my projects that doc with sphinx-immaterial theme. If you're building the theme from source then it requires node.js. But I see you installed it from the wheel, so it should be good without node.js.

2bndy5 commented 1 year ago

Looks like that monkey patch is trying to a CSS class for default values to args in py:function signatures: https://github.com/jbms/sphinx-immaterial/blob/main/sphinx_immaterial/apidoc/python/style_default_values_as_code.py#L9

2bndy5 commented 1 year ago

BTW, building on RTD timed out

Could this be due to the theme_comparison.py script? We ensure parallel-capable extensions are used in our docs. I have a feeling this is related to #738

2bndy5 commented 1 year ago

-b readthedocssinglehtmllocalmedia

IDK what builder this is, but we don't officially support anything other than HTML builders. We have our CI building with html and dirhtml builders, but not with singlehtml. We also overhauled the ToC generation to resemble that of mkdocs ToC (& for speed because render_partial() is detrimental).

2bndy5 commented 1 year ago

I found it. Looks like sphinx added an arg to better support multi-line signatures. Not sure if this was released for v7.0.1 though.

What can I do to avoid it?

You can't unless you use an older sphinx version, but that negates the change in docs/requirements.txt. We have to update the theme for v7.1 now...

mgeier commented 1 year ago

I re-tried it multiple times on RTD, and now it finally worked: https://readthedocs.org/projects/nbsphinx/builds/20769068/

Maybe it was just a temporary glitch.


And yes, I've also found the parse_arglist() change. This hasn't been released yet, but I'm using the master branch locally.

I just tried the latest Sphinx release, and there it works!

2bndy5 commented 1 year ago

Maybe it was just a temporary glitch.

Even more perplexing. 😕

I already started tracking the _parse_args() problem.

mgeier commented 1 year ago

Thanks!

I think I would like to clarify the comments, though: #746.