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

Collection of small changes resulting from final removal of plasmapy_sphinx from plasmapy #10

Merged
merged 8 commits into from
May 6, 2024

Conversation

rocco8773
Copy link
Member

@rocco8773 rocco8773 commented Apr 25, 2024

  1. fix typos
  2. copy over CSS changes

@rocco8773 rocco8773 changed the title Collection of small changes resulting from final removla of plasmapy_sphinx from plasmapy Collection of small changes resulting from final removal of plasmapy_sphinx from plasmapy May 6, 2024
@rocco8773
Copy link
Member Author

TL;DR We can NOT have the version flyout in the sidebar for our theme at the moment. It will have to live on the page like any theme that is not sphinx_rtd_theme.

@namurphy I think I have all the last changes needed from the migration from plasmapy. However, I've discovered an "issue" that is too big of a rabbit hole to try to "fix" at this time. This rabbit hole is associated with the version flyout menu.

As you're probably aware of, the flyout menu is generated in the sidebar when using sphinx_rtd_theme but gets placed on the bottom-right corner of the page when not using sphinx_rtd_theme. sphinx_rtd_theme does this using some JS magic and I found hack to make it work for our theme, but this hack does not work if we have RTD's (Beta) Addons enabled, which we do for plasmapy.

I tried implementing Addon's tag readthedocs-embed-flyout to inject the flyout, but this is poorly documented and does not appear to do anything during my prototyping.

Question: Do we want to live with the flyout location and continue with implementing plasmapy_theme into plasmapy? I think we should at least deploy plasmapy_sphinx as is.

@namurphy
Copy link
Member

namurphy commented May 6, 2024

@namurphy I think I have all the last changes needed from the migration from plasmapy. However, I've discovered an "issue" that is too big of a rabbit hole to try to "fix" at this time. This rabbit hole is associated with the version flyout menu.

Another Sphinx rabbit hole with a dragon in it... 🐰 🕳️ 🐉

Question: Do we want to live with the flyout location and continue with implementing plasmapy_theme into plasmapy? I think we should at least deploy plasmapy_sphinx as is.

This is preserving the status quo, I think? I agree we should deploy plasmapy_sphinx as is, and save the fixes to this problem for later.

Also: thank you so for doing this!

@namurphy
Copy link
Member

namurphy commented May 6, 2024

Also I'm planning on doing a release tomorrow or Wednesday (the deadline for including the newest version in the Python in Heliophysics Community summer school). Would it be okay to merge the full removal of plasmapy_sphinx after I do that release? I'm thinking it'd be safer to delay it in case this change causes any more Sphinx rabbit dragon holes... (or maybe Sphinx rabbit hole dragons?)

The official release of plasmapy_sphinx does not need to wait, though.

@rocco8773
Copy link
Member Author

Yes, go ahead and release plasmapy without the removal of plasmapy_sphinx.

@rocco8773
Copy link
Member Author

Question: Do we want to live with the flyout location and continue with implementing plasmapy_theme into plasmapy? I think we should at least deploy plasmapy_sphinx as is.

This is preserving the status quo, I think? I agree we should deploy plasmapy_sphinx as is, and save the fixes to this problem for later.

Yes and no. Yes, this is how the flyout has behaved for non-sphinx_rtd_theme's since the beginning of time. No, we have been using sphinx_rtd_theme in plasmapy, so our flyout will change locations when we implement plasmapy_theme.

@rocco8773 rocco8773 merged commit 5943a54 into main May 6, 2024
5 checks passed
@rocco8773 rocco8773 deleted the last_changes_for_plasmapy_removal branch May 6, 2024 22:54
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.

2 participants