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

Fix css for 'sphinx_copybutton' #349

Merged
merged 31 commits into from
Jan 9, 2020

Conversation

s-weigand
Copy link
Contributor

Changed CSS so 'sphinx_copybutton' will look properly out of the box.

  • Moved padding: 0.4em; from .input_area, .output_area to its child pre
  • Added padding: 0.4em; to other .input_area, .output_area child elements, to keep the styling consistent
  • Added sphinx_copybutton to the docs
  • Tested output for alabaster and sphinx_rtd_theme

Before:
copybtn_before

After:
copybtn_after

After sphinx_rtd_theme:
copybtn_after_rtd

closes #333

src/nbsphinx.py Outdated Show resolved Hide resolved
@mgeier
Copy link
Member

mgeier commented Dec 5, 2019

Thanks a lot for this PR, and sorry for the late reply!

As long as the extension is not properly supported on most themes (or at least on the RTD theme), I hesitate to enable it in the nbsphinx docs (because they are available for many themes).

What about just adding the CSS fixes?

@s-weigand
Copy link
Contributor Author

Some remarks after working on this:

  1. Changing styles without changing styles and by hand doing side by side compares is a tedious job and I found this service helpfull for the job (5k screenshot compares/month free for open source and you could just upload the full pages from the docs).
  2. IMHO it would be more handy to move the css to an actual *.css file for linting + autocomplete and just read it to a string in the package (same would be good for the RST and LaTeX templates).

Just some ideas 😄

@s-weigand
Copy link
Contributor Author

s-weigand commented Dec 6, 2019

Hmmm looks like 'Python 2.7 + latest Sphinx' has some problems with hashlib on mac 😵
Maybe deleting the cache will help.

@mgeier
Copy link
Member

mgeier commented Dec 9, 2019

I've fixed the Travis-CI failure in #363.

@mgeier
Copy link
Member

mgeier commented Dec 9, 2019

Changing styles without changing styles and by hand doing side by side compares is a tedious job and I found this service helpfull for the job (5k screenshot compares/month free for open source and you could just upload the full pages from the docs).

I'm only open to free-and-unlimited-for-open-source services.

I don't like if there is a hard limit. Though admittedly, 5k seems quite a lot.

Anyway, I have no clue how much work it would be to set something like this up.

IMHO it would be more handy to move the css to an actual *.css file for linting + autocomplete and just read it to a string in the package (same would be good for the RST and LaTeX templates).

Yes, I agree.
The files don't even have to be read into a string, there should be officially supported ways to include those files in the Sphinx build process. I didn't really look into that, though.

I started with everything in one file, when everything was still small. Now I think it is a good idea to make separate files at some point, I just didn't really have the motivation to do so.

If you like, you can make a PR!

src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
@s-weigand s-weigand force-pushed the fix-sphinx_copybutton-css branch from c3f8c19 to 57ac71e Compare December 10, 2019 18:15
@s-weigand
Copy link
Contributor Author

s-weigand commented Dec 19, 2019

Hmmm repology is down atm, which is why the tests fail.
With the CSS as is atm, the style of some warning boxes changes, but I can't reproduce the warnings.

$ jupyter lab --version
1.2.4

Is it a IPython or nbconvert thing?

@mgeier
Copy link
Member

mgeier commented Dec 19, 2019

Repology seems to be up again (repology/repology-webapp#101)!

With the CSS as is atm, the style of some warning boxes changes, but I can't reproduce the warnings.

That's not a problem.

JupyterLab and the Classic Notebook don't display anything in this case, but nbsphinx does.
IMHO it's nice to see that something is wrong. But it will happen quite rarely and the styling of these warnings is not important.

.gitignore Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
@mgeier
Copy link
Member

mgeier commented Dec 26, 2019

Thanks for the updates!

Please note my attempted CSS changes there: https://github.com/choldgraf/sphinx-copybutton/pull/45

src/nbsphinx.py Outdated Show resolved Hide resolved
Copy link
Member

@mgeier mgeier left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

Now this PR is nicely compact, I like it.

I think I will wait until sphinx_copybutton makes a new release and will merge this PR afterwards.

doc/conf.py Outdated Show resolved Hide resolved
src/nbsphinx.py Outdated Show resolved Hide resolved
@mgeier
Copy link
Member

mgeier commented Jan 3, 2020

Thanks!

There's still a problem with the placement of prompts, see #349 (comment).

src/nbsphinx.py Outdated Show resolved Hide resolved
@mgeier
Copy link
Member

mgeier commented Jan 4, 2020

Thanks for the update! Now the only thing that's missing is setting the prompt padding to 0.3rem.

Here is the current rendered version, using the latest release of sphinx_copybutton: https://155-46379698-gh.circle-artifacts.com/0/html/code-cells.html

@s-weigand s-weigand force-pushed the fix-sphinx_copybutton-css branch from 0224b91 to c1bb0a6 Compare January 8, 2020 18:17
'nightly' python to 3.8 since nightly python now is 3.9
@s-weigand s-weigand force-pushed the fix-sphinx_copybutton-css branch from c1bb0a6 to 4e935c3 Compare January 8, 2020 18:21
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@mgeier mgeier merged commit aad12aa into spatialaudio:master Jan 9, 2020
@mgeier
Copy link
Member

mgeier commented Jan 9, 2020

Thanks a lot for your work!

@s-weigand s-weigand deleted the fix-sphinx_copybutton-css branch January 9, 2020 19:02
@s-weigand
Copy link
Contributor Author

Thanks for all the detailed reviews 😄
Next I might look into pulling the template strings into separate files, with the proper extension.

@mgeier
Copy link
Member

mgeier commented Jan 9, 2020

Cool, I'm looking forward to this!

BTW, I've re-based all theme branches to see if the copy button works.
See https://nbsphinx.readthedocs.io/en/latest/usage.html#HTML-Themes for a list of themes.

I've found some issues in a few themes: #376

But all in all it looks great in most themes!

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.

The sphinx_copybutton extension doesn't properly work out-of-the-box
3 participants