Skip to content

Conversation

@sneakers-the-rat
Copy link
Contributor

@sneakers-the-rat sneakers-the-rat commented Jan 31, 2025

Fix: #523

Collapsible cells currently look like this:

sphinx_book_theme furo pydata_sphinx_theme alabaster sphinx_rtd_theme sphinx_immaterial
Screenshot 2025-01-30 at 10 45 06 PM Screenshot 2025-01-30 at 10 46 52 PM Screenshot 2025-01-30 at 10 47 38 PM Screenshot 2025-01-30 at 10 48 17 PM Screenshot 2025-01-30 at 10 49 19 PM Screenshot 2025-01-30 at 10 50 41 PM

biggest problem here is that they don't handle dark mode. secondary is that they don't adapt to the theme. this means that myst-nb is not really usable out of the box for all the modern themes that have a dark mode! I've been copy/pasting the same dark mode overrides for myst-nb sphinx projects for years and i figure i might as well try to help out with the problem.

challenge: there isn't a standard way to declare colors as css vars or things we could otherwise anticipate and use when rendering a notebook - we can't reliably ship opinionated css for rendering collapsibles (and it's arguably not our job, but the job of the theme).

proposal: use admonition classes! Very simple change to the html rendering process to add classes and switch a span to a p, and then clearing out the css rules that set colors, add some rules that unset values commonly used with details and admonition tags so they take on the appearance of the theme.

sphinx_book_theme furo pydata_sphinx_theme alabaster sphinx_rtd_theme sphinx_immaterial
closed Screenshot 2025-01-30 at 10 18 07 PM Screenshot 2025-01-30 at 10 08 17 PM Screenshot 2025-01-30 at 10 19 58 PM Screenshot 2025-01-30 at 10 21 35 PM Screenshot 2025-01-30 at 10 23 19 PM Screenshot 2025-01-30 at 10 25 27 PM
open Screenshot 2025-01-30 at 10 18 27 PM Screenshot 2025-01-30 at 10 08 46 PM Screenshot 2025-01-30 at 10 20 14 PM Screenshot 2025-01-30 at 10 21 51 PM Screenshot 2025-01-30 at 10 23 38 PM Screenshot 2025-01-30 at 10 25 41 PM

so not 100% perfect, but imo an improvement. only breaking change to existing use will be to swap out span -> p in css overrides, but this should also give us a more stable selector to target in the future rather than needing to override by being relative to a .cell element (we could add a more unique class just for the sake of overriding, but didn't want to do too much)

edit: a more foolproof version of this would be to not use details but directly mimic the structure of admonitions and use javascript to open/close the container. i could also do that if maintainers prefer, i just figured start with this since it was easier and more semantic html5 compliant, but it does miss the styles a bit in some of these themes.

@sneakers-the-rat
Copy link
Contributor Author

I don't think these test failures are related to this PR? some pyarrow bug?

also sorry this is i think the second time i have raised a PR without an issue discussion first, i just saw a sort of low-hanging fruit option and took it. but obvi open to closing this and pursuing any other strategy

@bsipocz
Copy link
Collaborator

bsipocz commented Feb 3, 2025

@sneakers-the-rat - thanks for the contribution!

Re CI: -Yes, we started seeing some CI issues in the scheduled runs, too. I haven't had the time to look into those.

As for your PR, this will need a review from someone who is more familiar with the frontend parts of the package.

@bsipocz bsipocz added the bug Something isn't working label Apr 25, 2025
Copy link
Collaborator

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Since this clearly improves the behaviour for the bug that many downstream already patched over, I would go ahead and merge it.

If anyone runs into problems because of it, do feel free to open an issue.

@bsipocz bsipocz merged commit 9bccf22 into executablebooks:master Apr 25, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect "show/hide code cell output" background in dark mode

2 participants