IMPROVE: Harmonize themes between static and live code

[patrickmineault]

As mentioned in executablebooks/sphinx-thebe#39, there is a discrepancy between how static code and live code look in the sphinx book theme. This PR patches CodeMirror CSS to match the pygments css. It's not perfect, but it's hard to distinguish the two with the naked eye.

Before, thebe.md (static on top, live at bottom):

After:

Seeking guidance: right now the CSS is patched at the worst possible spot in the footer - would like to know where to put it in, or whether to split off the CSS in a couple of different places. Also would like to know if the test thebe.md file should be put in the docts or put in a test folder somewhere.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

[choldgraf]

Thanks for the PR! A few comments:

• Can you add the extra page to a toctree so that Sphinx doesn't emit a warning, and then ReadTheDocs will be able to build the preview: https://readthedocs.org/projects/sphinx-book-theme/builds/14625292/
• How dynamic is the CodeMirror DOM? Does it change often? What about when CodeMirror6 comes out (which I imagine is relatively soon)?

I think the main thing I'm worried about is that this is some special-case CSS that will be very hard to notice/test if there's a regression.

I also want to throw out one other possibility that didn't come to mind before: what if we went in the other direction, and instead of making the CodeMirror CSS match the book theme CSS, we made minor modifications to the book theme code cells CSS to make it match one of the more popular CodeMirror styles?

[patrickmineault]

1. Done. I've moved the test page to the kitchen sink.
2. CM6 uses a different styling mechanism than CM5. However, there's a property, classHighlightStyle, that permits styling through CSS. It would be a simple matter of adding support for CM6 this way.

There's a lot more CSS classes in pygments than in CM5, so it was simpler to write it in this direction. If there's good reason to do the reverse, that would also work. I do think that static code is far more used than live code, and the CM -> pygments route would affect many more downstream people, so I think the surface is smaller for CM -> pygments.

[choldgraf]

I think that this looks good to me - just one quick comment to add more context for folks.

One thing we should flag is if we ever change the static code cell style, we'll also need to update these styles as a result. One case where I could imagine this happening is if we use a "dark mode toggle" that might affect code cells as well.

[patrickmineault]

Just one thing before - what would be the ideal location to put the link to the stylesheet? Right now it's in footer.html, there is probably a better spot for it.

[choldgraf]

@patrickmineault hmmm good question, this relates to another thought that I had as well actually.

If we bundle this CSS then will it clobber any theme changes that people select for CodeMirror? If somebody explicitly says "I want a specific CodeMirror theme" then I think that we should make sure they get it, not force them to use this style. Answering that question might impact where this CSS gets loaded

[patrickmineault]

@choldgraf good point - I will check how this integrates with the codemirror_theme config in jupyterbook and get back to you.

[choldgraf]

cool - perhaps we could just add some conditional logic that checks whether sphinx-thebe has an explicit user configuration for the CodeMirror theme, and if so, then doesn't load this CSS

[patrickmineault]

I found an issue trying to integrate with jupyterbook: it turns out pygments_style is tango in sphinx-book-theme and sphinx in jupyterbook. So it's not possible to fully resolve the discrepancy between pygments and codemirror across both with one static style sheet. What do you think is the path forward here, given the likely support for dark theme in the near future?

[choldgraf]

huh - that is strange...did you find where they were configured? I would have assumed that Jupyter Book just inherits whatever comes from the book theme. My feeling is that we should modify Jupyter Book to just "use whatever the default in the book theme is". Other themes may want to define their own behavior with pygments, so that shouldn't be defined at the Jupyter Book level IMO

[patrickmineault]

In sphinx-book-theme, it's defined in src/jinja/theme.conf.j2, while in jupyter-book, it's defined in jupyter_book/config.py. The key is pygments_style.

I thought I could define which one is the canonical styling as whichever one matches jupyterlab. However, it turns out jupyterlab uses a 4th style unlike the first three encountered so far:

I'm starting to think that maybe this is a rabbit hole I shouldn't go down further. What I could do instead is remove the attempt to change the colors, only fix the spacing and font, and add a little bit of CSS so that when you have a cell highlighted it does the same as in jupyterlab, that is make the cell background white and put an outline around it.

[patrickmineault]

Ok, how about this? This improves the match of the look of the live code cells compared to static code cells. It also improves the usability of the code cell by changing the background to white and putting an outline around the cell. I doesn't attempt to match the colors, since this appears to open up a can of worms.

[choldgraf]

I think that this looks quite nice - and avoids us needing to deal with the extra complexity of special-casing theme choices and such. I'm +1 on it if you think that this still works for you :-)

[patrickmineault]

Yeah, this is def an improvement! I've moved all the css to _page.scss, I think this should be good to go once it's all rebuilt.

[choldgraf]

I think this looks really nice - it is a good compromise between extra complexity and making obvious incremental improvements. I'll merge it in once the tests are happy!

[welcome]

Congrats on your first merged pull request in this project! 🎉


Thank you for contributing, we are very proud of you! ❤️

[choldgraf]

thanks for this improvement!