spatialaudio/nbsphinx

Problematic themes for sphinx_copybutton

Opened this issue · 10 comments

mgeier commented

It has yet to be determined for each of those cases if the problem has to be fixed in the theme itself, in sphinx_copybutton or here in nbsphinx.

A separate issue/PR should be made for each of these:

In multiple themes the tooltip font is too large or too small. This is not a biggie, but maybe it can be improved for some themes.

s-weigand commented

Guess most things can be fixed by explicitly setting margin: 0 in this css block.

itcase does'n have copybutton.css in its body at all, for some reason.
But the copybutton.js and copy-button.svg are present, so I guess the theme does some filtering and messes things up.

mgeier commented

@s-weigand Would you mind creating an issue or PR for https://github.com/choldgraf/sphinx-copybutton?

The itcase case is indeed interesting, because the file itself is there (https://nbsphinx.readthedocs.io/en/itcase-theme/_static/copybutton.css), it's just not included in the HTML.
Anyway, this doesn't have a high priority for me, I don't know if anyone is even using this theme.
I guess we can just wait and see until somebody complains.

[UPDATE: I've created ITCase/itcase_sphinx_theme#38]

s-weigand commented

I can do this, but first I will create a tool to generate all the different theme docs in one swoop.
Working on #349 it already git kinda annoying to comment and uncomment html_theme = 'sphinx_rtd_theme' in doc/conf.py and not accidently pushing the changes.

I got a first draft/idea for this, which would work as follows:

  1. Copy doc in a temp folder
  2. Add temp git remote (git@github.com:spatialaudio/nbsphinx.git)
  3. Extract diff of doc/conf.py on master and on <*-theme>-branch
  4. Add the diff to the temp conf.py
  5. Build theme docs in a folder with the theme-branchname
  6. Remove temp git remote

Messing with git (via GitPython) isn't too pretty, but IMHO the most elegant way to prevent duplication and the resulting maintenance.

What are your thoughts on this? Do you think this is worth a shot for some dev comfy?

P.S.: The diff extraction allready works 😄

mgeier commented

I can do this, but first I will create a tool to generate all the different theme docs in one swoop.

That's a really good idea!

I've just manually (and temporarily) re-based the theme branches onto the work-in-progress branch to try different themes.
But this is of course annoying. Would be great to have a script that allows quicker iteration on this.

I've made a script for re-basing all theme remotes: https://github.com/spatialaudio/nbsphinx/blob/master/git_rebase_theme_branches.sh

Something similar for local usage would be great!

Copy doc in a temp folder

Instead of copying, you could probably use git workdir (https://git-scm.com/docs/git-worktree)?
I've never used this, but it sounds like it could be helpful.

Extract diff [...] Add the diff

This sounds just like rebasing. This way, Git can do all the diffing for you.

Add temp git remote [...] Remove temp git remote

I normally have the "spatialaudio" URL as origin and my own fork as a separate remote.
If we assume origin to be the "upstream" remote, I think there is no need to add and remove a new remote in the script.

If you prefer, we can also change the name to upstream.

[...] GitPython [...]

I've never used GitPython, but if you think it's a good tool for this, I'm fine with it.
I would probably try it with a Shell script first, but a Python script would of course be more platform-independent.

Do you think this is worth a shot for some dev comfy?

Definitely!

When running Sphinx on all those different theme directories, it might be a good idea to use the same "doctree directory" for all of them (using the -d command line option). If that works, it might reduce the compilation time dramatically (because the notebooks have to be executed just once instead of once per theme).

s-weigand commented

I made a PR at sphinx-copybutton, which should fix all but the cloud-theme issue on the sphinx-copybutton side. Since I can't find the source of cloud-theme I have no idea what is happening with the icon (my guess is that this is some js stuff).
For julia-theme, pytorch-theme, itcase-theme and pyviz-theme the problem is due to the theme template, but my changes in the PR should restore the copy functionality for julia-theme and pytorch-theme.

mgeier commented

Thanks!

Since I can't find the source of cloud-theme I have no idea what is happening with the icon (my guess is that this is some js stuff).

This is the source: https://bitbucket.org/ecollins/cloud_sptheme/

s-weigand commented

Can't figure out what is going wrong in cloud-theme. On chrome it is even worse, if you just hold the left mouse button down, you get Schrödingers copybutton, it's on the left and the right side at the same time. My guess is that it has something todo with the Pseudoelement ::after, which is used for the tooltip.

When I tried:

::after{
  display: none;
}

The Schrödingers copybutton did stop and it was left on 'mouse down+hover' and right else.

Also the binder badge in pytorch-theme is massive, I think this should be changed in the css of nbsphinx or the implementation in doc/conf.py.
I.e. this would do the trick:

img[alt="Binder badge"]{
  width: unset;
}

The inline style of the img tag (vertical-align:text-bottom) could be moved to that rule as well.
Or width: unset; could be made an inline style.

mgeier commented

The Schrödinger behavior of the cloud theme is indeed strange. I have no idea what's happening there.
But this really isn't an nbsphinx problem. Would you mind creating an issue at the tracker of either the cloud theme or at sphinx-copybutton (whatever you think is more appropriate)?

The Binder badge is not part of nbsphinx (since it is created entirely by user settings in conf.py), therefore there should be nothing added to the nbsphinx CSS.
I think this should be done inline if necessary.
But I'm wondering: why is this only happening in one theme and all others are fine?

s-weigand commented

I will open a ticket at cloud-theme, since sphinx-copybutton works fine on other themes.

The other themes, as I saw it, set max-width: 100%;, but pytorch sets width directly.
Which is also why most other images are stretched.

article.pytorch-article img {
    width: 100%;
}

IMHO having the width: unset; in the conf.py would be nice, since it's an example configuration, which most users will just copy and paste from.

mgeier commented

The other themes, as I saw it, set max-width: 100%;, but pytorch sets width directly.

So this is a bug in the pytorch theme?

IMHO having the width: unset; in the conf.py would be nice, since it's an example configuration, which most users will just copy and paste from.

If that's just a work-around for the bug mentioned above, I would prefer to fix the bug instead.