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

hide copy-button in sphinx-immaterial theme #737

Merged
merged 2 commits into from May 24, 2023
Merged

hide copy-button in sphinx-immaterial theme #737

merged 2 commits into from May 24, 2023

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented May 2, 2023

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 #737 (comment) for a detailed analysis.

@mgeier
Copy link
Member

mgeier commented May 4, 2023

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
Copy link
Contributor Author

2bndy5 commented May 4, 2023

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
Copy link
Contributor Author

2bndy5 commented May 4, 2023

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
Copy link
Contributor Author

2bndy5 commented May 4, 2023

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

requires = ["setuptools >= 40.8.0"]

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

@2bndy5 2bndy5 changed the title Inline literal exec cnts hide copy-button in sphinx-immaterial theme May 4, 2023
@mgeier
Copy link
Member

mgeier commented May 21, 2023

Not exactly sure why the RTD build failed.

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

uses extra specificity to overrides theme-specific CSS
@mgeier
Copy link
Member

mgeier commented May 21, 2023

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
Copy link
Contributor Author

2bndy5 commented May 21, 2023

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
Copy link
Contributor Author

2bndy5 commented May 21, 2023

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
Copy link
Contributor Author

2bndy5 commented May 21, 2023

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
Copy link
Contributor Author

2bndy5 commented May 21, 2023

-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
Copy link
Contributor Author

2bndy5 commented May 21, 2023

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
Copy link
Member

mgeier commented May 21, 2023

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
Copy link
Contributor Author

2bndy5 commented May 21, 2023

Maybe it was just a temporary glitch.

Even more perplexing. 😕

I already started tracking the _parse_args() problem.

@mgeier mgeier merged commit 05957b0 into spatialaudio:master May 24, 2023
4 checks passed
@mgeier
Copy link
Member

mgeier commented May 24, 2023

Thanks!

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

@2bndy5 2bndy5 deleted the inline-literal-exec-cnts branch May 25, 2023 01:51
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

Successfully merging this pull request may close these issues.

use inline literal instead of a literal_block in execution counts
2 participants