-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
use sphinx-design and theme's light/dark image selectors #10671
Conversation
Circle failure reported upstream: mcmtroffaes/pybtex-docutils#21 |
In the meantime, it's possible that adding |
@mcmtroffaes is usually very quick, let's wait a day and see rather than hastily doing a version-pin or adding a warning-ignore |
Another failure, reported upstream: numpy/numpydoc#399 seems like using sphinx 5.0 beta (which allows docutils 0.18) is behind these. |
ca4c2b8
to
f6664b7
Compare
dang, forgot to add |
|
@drammock can you please ping me once it builds? I can then have a look and will review :) thanks! |
argh, sorry, seems I ran my |
_assemble(node, self) | ||
return [node] | ||
|
||
|
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.
No suitable replacement for .. details::
in sphinx-design?
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.
hmm, seems like it wasn't even being used:
$ git grep ".. details::"
doc/sphinxext/sphinx_bootstrap_divs/__init__.py:70:# .. details::
doc/sphinxext/sphinx_bootstrap_divs/__init__.py:73: """Class for .. details:: directive."""
I think that means we can delete the entire sphinx-bootstrap-divs
extension?
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.
Yes, feel free!
doc/install/manual_install.rst
Outdated
$ mamba create --override-channels --channel=conda-forge --name=mne mne | ||
|
||
|
||
.. tab-item:: macOS (M1 / Apple Silicon) |
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.
This entire bit is already gone in main
and has been backported
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.
dang, I must have messed up the rebase this morning. thanks for catching that. I just pushed a fix.
.. collapse:: |hand-paper| If you get an error... | ||
:class: danger | ||
.. dropdown:: If you get an error... | ||
:icon: alert-fill |
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.
here (and in other dropdowns) we can also later add things like :color: warning
or :color: danger
, but at present they don't work yet; see executablebooks/sphinx-design#70
doc/install/installers.rst
Outdated
.. button-link:: https://github.com/mne-tools/mne-installers/releases/download/v1.0.3/MNE-Python-1.0.3_0-macOS_Intel.pkg | ||
:ref-type: ref | ||
:color: primary | ||
:shadow: | ||
:class: font-weight-bold | ||
|
||
|cloud-download-alt| |ensp| Download for macOS | ||
|
||
**Supported platforms:** | ||
macOS 10.15 (Catalina) and newer (Intel and Apple Silicon) |
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.
this is where the changes in #10693 would end up I think
let all_tab_nodes = document.querySelectorAll( | ||
'.platform-selector-tabset')[0].children; | ||
let input_nodes = [...all_tab_nodes].filter( | ||
child => child.nodeName === "INPUT"); | ||
let tab_label_nodes = [...document.querySelectorAll('.sd-tab-label')]; | ||
let correct_label = tab_label_nodes.filter( | ||
label => label.textContent.trim().toLowerCase() === platform)[0]; | ||
let hash = correct_label.getAttribute('for'); | ||
let correct_input = input_nodes.filter(node => node.id === hash)[0]; | ||
correct_input.checked = true; | ||
}); |
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.
there's probably a more elegant way to code this in JS, maybe @hoechenberger knows? Crossref for why it's so convoluted: executablebooks/sphinx-design#68
_assemble(node, self) | ||
return [node] | ||
|
||
|
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.
hmm, seems like it wasn't even being used:
$ git grep ".. details::"
doc/sphinxext/sphinx_bootstrap_divs/__init__.py:70:# .. details::
doc/sphinxext/sphinx_bootstrap_divs/__init__.py:73: """Class for .. details:: directive."""
I think that means we can delete the entire sphinx-bootstrap-divs
extension?
doc/install/index.rst
Outdated
:shadow: | ||
:class: font-weight-bold | ||
|
||
|cloud-download-alt| |ensp| Download Installers |
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.
This (the cloud-download-alt
) currently doesn't render correctly, probably should just get removed for now. Upstream issue: executablebooks/sphinx-design#66
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.
Never mind my comments below, then! Feel free to comment out the icons for now then
(fontawesome HTML is in the buttons) |
yep, I noted that in my review. Question is whether to wait for upstream fix, or live without the icons for now |
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.
@hoechenberger feel free to merge if you're happy!
I will take a look tomorrow! In any case we should probably not merge until we have native Apple Silicon installers ready, no? I believe will get there until Friday. |
I think it's easiest to merge this, then deal with M1 whenever it's ready |
Ah sorry I was mistaken, I thought the phrasing in this PR already assumed we had native M1 builds. Seems my brain is a bit tired :) I'll review tomorrow morning CEST and merge, ok? :) |
I added a bit morge top-margin, otherwise all looks great to me! fantastic work! |
The Circle error is related to some setup problem. I successfully built with my changes locally. Going ahead and merging this one. |
Thanks @drammock!! |
No description provided.