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

Switch to MDA rtd sphinx theme #122

Merged
merged 14 commits into from
Jan 4, 2024
Merged

Switch to MDA rtd sphinx theme #122

merged 14 commits into from
Jan 4, 2024

Conversation

ojeda-e
Copy link
Member

@ojeda-e ojeda-e commented Jan 3, 2024

As we agreed with @IAlibay, I am here submitting changes to update the docs theme to the official one. This PR fixes #120 and #110 (opened by @orbeckst).

Description

Update docs mdanalysis-sphinx-theme with plus maintenance.

Status

  • Ready to go

It compiles locally and seems to work as expected. I'll be fixing the issues if any once it build in the CI.
Thanks.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Merging #122 (b931fad) into main (0cf4856) will not change coverage.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Thanks for link fixing and face-lifting.

Is the content from the removed notebooks available elsewhere?

@ojeda-e
Copy link
Member Author

ojeda-e commented Jan 3, 2024

Is the content from the removed notebooks available elsewhere?

@orbeckst I just noticed the diff for the notebooks are misleading. The notebooks are still part of the documentation. I had to regenerate them to verify every cell works and the output is correct -the same as before-; the nb kernel was also updated to enable docs build in the CI.
The content is available in the reviewnb app: https://app.reviewnb.com/MDAnalysis/membrane-curvature/pull/122/

Here are the build notebooks in the tutorial section in case you want to check:

@orbeckst orbeckst linked an issue Jan 3, 2024 that may be closed by this pull request
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for putting in the effort!

@ojeda-e ojeda-e mentioned this pull request Jan 3, 2024
@ojeda-e
Copy link
Member Author

ojeda-e commented Jan 3, 2024

@lilyminium, if possible, would you please help me with a quick review here before I merge?

@IAlibay
Copy link
Member

IAlibay commented Jan 3, 2024

@ojeda-e apologies I am unlikely to have much time to look at this until the weekeend - please do re-ping me if you still need my input by then.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

Looks pretty good @ojeda-e, I've just left some comments largely on the notes. I also think you've put last year in the "Last executed" line of docs/source/pages/Curvature_membrane-only_systems.ipynb (ReviewNB won't let me comment directly....)

Just commenting this review to avoid blocking progress :-)

docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/requirements.txt Outdated Show resolved Hide resolved
@ojeda-e
Copy link
Member Author

ojeda-e commented Jan 4, 2024

Thanks for taking the time here @lilyminium
I addressed your comments. Hopefully I didn't miss anything. Removed the editable version part, removed Sphinx from the requirements and fixed the date on the notebook.It's a bit late here, but there if there are more comments please feel free to leave them and I'll address them tomorrow.

@ojeda-e ojeda-e merged commit c172bc7 into main Jan 4, 2024
16 checks passed
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.

Add MDAnalysis sphinx theme link to blog post in docs broken
4 participants