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

Test new theme capabilities #9916

Merged
merged 12 commits into from
Jan 6, 2022

Conversation

drammock
Copy link
Member

This PR tests whether the proposed version switcher being built into our theme will work cleanly as a replacement for our custom version switcher. #9915 already added the JSON file; this PR uses the WIP theme version in pydata/pydata-sphinx-theme#436. Along the way it removes some custom CSS that is no longer needed (because the theme has improved to handle those elements better).

@larsoner
Copy link
Member

+1 for switching once there is a suitable upstream commit we can pin to (or wait for a release if they're quick)

@drammock
Copy link
Member Author

drammock commented Oct 28, 2021

OK, it basically works. The dropdown links aim for the corresponding page in the docs instead of always going to the root page of that version of the docs, which is better than what we had before.

The one thing that we lose is the contextual coloring of the button (before we had a blue button for stable docs pages and a red one for dev docs pages). I can live with that loss, it wasn't that important.

I'll leave this open (but in Draft mode) until the theme cuts a new release, at which point we change requirements_doc.txt and hopefully everything still works.

@larsoner
Copy link
Member

If you add an option to specify a css class in the config json that ends up in the html, it would give people a way to set colors

@drammock drammock force-pushed the test-pst-version-switcher branch 2 times, most recently from 197213b to 3d7e162 Compare November 3, 2021 16:20
@drammock
Copy link
Member Author

drammock commented Dec 3, 2021

TestSphinxWindows failure has been reported upstream: pydata/pydata-sphinx-theme#523

@larsoner WDYT, should we merge this anyway? The docs are built on a linux instance and the switcher looks good in the CI artifact: https://39352-1301584-gh.circle-artifacts.com/0/dev/install/contributing.html

NB: in the CI artifact it's using the JSON file currently on main, which is why it shows 0.24 as "devel". This PR also updates that JSON so after merge-build-deploy it should correctly show 1.0 (devel) and 0.24 (stable)

@drammock drammock marked this pull request as ready for review December 3, 2021 22:49
@larsoner
Copy link
Member

larsoner commented Dec 4, 2021

We don't want red CIs to mean "might still be okay to merge" -- it leads to too much cognitive burden and risk about "what's an okay failure", and "did I make sure it's only the right build that failed". If it's an upstream Windows bug that we're willing to temporarily ignore, let's either disable the windows sphinx build or (probably better, I think it's possible?) mark it as an allowed failure in the CI matrix at the Azure config end so that the merge button stays green but the build shows up gray on the GitHub interface.

@drammock
Copy link
Member Author

drammock commented Dec 4, 2021

Ok. This PR isn't urgent, let's give upstream a few days before we go disabling tests.

@drammock
Copy link
Member Author

drammock commented Dec 5, 2021

upstream fix is in, restarting the failed CI.

@drammock
Copy link
Member Author

drammock commented Dec 5, 2021

different failure now. I'll look next week.

@drammock
Copy link
Member Author

drammock commented Jan 4, 2022

second upstream fix is in (pradyunsg/sphinx-theme-builder@85be6e9) so hopefully the azure build-docs-on-windows job will be green now

@drammock
Copy link
Member Author

drammock commented Jan 4, 2022

ok the SphinxWindows job succeded. Ready for merge when remaining CIs come back green.

@larsoner larsoner merged commit 4679e87 into mne-tools:main Jan 6, 2022
@larsoner
Copy link
Member

larsoner commented Jan 6, 2022

Thanks @drammock !

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.

None yet

2 participants