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 admonition-sidebar styling #847
Conversation
here's the rendered doc section: https://pydata-sphinx-theme--847.org.readthedocs.build/en/847/user_guide/theme-elements.html#admonition-sidebars IMO this is basically a bugfix, so I'll plan to merge tomorrow morning (Chicago time) unless there are objections. |
|
||
// No margin since we have less horizontal space w/ the sidebar | ||
&.attention, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this not inherit the border color from other CSS rules? I would have thought that happened. It feels like we are not being DRY by having this duplicated in both, but it's not a huge deal. Just want to check that these are definitely necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed it does not. Without this, the border is white.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add that I also expected it to inherit the correct color, and I'm not sure why it doesn't. Clearly I have not yet snatched the pebble from the CSS master's hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what version of docutils are you using? I just tried cloning this PR locally and deleting all of these sections, and the colors showed up as usual in the sidebar areas, so maybe there's a docutils difference here and we should figure out if that's a regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on 0.18.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, i figured it out. my docs-live
env has sphinx 4.2.0 and docutils 0.17.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is probably why I was sometimes seeing different results when building the PST documentation vs building MNE-Python docs... they were building under different sphinx/docutils versions (meaning maybe some of the HTML tags/classes were actually different?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that we want to still support sphinx 4 / docutils 0.17 (and thus if we need extra rules to make it look the same under sphinx 4 vs sphinx 5, then we should add those extra rules). That is, until sphinx 5 is at least 6 months old. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that makes sense to me - maybe we can just add a comment there like // TODO: check that these rules are still needed after we deprecate Sphinx 4
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in #848
closes #846