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

docs: change the theme #238

Merged
merged 11 commits into from
Aug 25, 2023
Merged

docs: change the theme #238

merged 11 commits into from
Aug 25, 2023

Conversation

12rambau
Copy link
Contributor

@12rambau 12rambau commented Jan 7, 2023

@mariobuikhuizen, I would like to work on the documentation and update the template that is used. my objective are the following :

  • use the pydata-sphinx-theme (because thats my favorite for sphinx)
  • refactor the structure of the documentation to expose the API
  • try to pull information from the vuetify API
  • use the widgetti logo

This PR is just a demo using pydata-sphinx-theme, is there a chance to see this PR merged if I continue working on it ?

Related to #268

@maartenbreddels
Copy link
Collaborator

I like the theme and your plans!

@12rambau
Copy link
Contributor Author

requires #240 to move forward

@maartenbreddels maartenbreddels force-pushed the master branch 3 times, most recently from 368ca6d to e241012 Compare April 3, 2023 15:04
@maartenbreddels maartenbreddels force-pushed the master branch 5 times, most recently from 0017486 to f4937c0 Compare April 18, 2023 19:22
@12rambau
Copy link
Contributor Author

12rambau commented Aug 22, 2023

@mariobuikhuizen I'll stop this PR here to avoid a too big scope. what is cover is the following:

  • adapt deps to use the pydata-sphinx-theme
  • adapt conf.py accordingly (and remove automatically generated comments that are not there anymore since Sphinx 6)
  • refactor the toctree to make it conform to pydata-sphinx-theme

work that I will do in the following-up PR: reorder pages to make better use of the theme sidebars.

In case you think this should not be put in production yet (to which I agree) you should publish a stable version of the docs in RDT and link it in the readme. I think you need to make a GitHub release for that, else you can manually trigger the rebuild of stable. Let me know what you prefer.

@12rambau 12rambau marked this pull request as ready for review August 22, 2023 20:38
@mariobuikhuizen
Copy link
Collaborator

mariobuikhuizen commented Aug 24, 2023

Thank you for not increasing the scope further! Can you please remove the changes not related to changing the theme?

@12rambau
Copy link
Contributor Author

I scanned the modifications again and I see 2 potential candidates:

  • drop of requirements.txt (not used anywhere since build: use nox for tests and docs  #240)
  • drop the extensive comments present in the conf.py when I refactored it to include the pydata-sphinx-theme config.

Which should I remove ?

@mariobuikhuizen
Copy link
Collaborator

To be clear, those changes are good to do, but not in the PR called "change the theme".

@12rambau
Copy link
Contributor Author

then the answer is "both of them" 😄 Understood, I'll create other PR for them

@mariobuikhuizen
Copy link
Collaborator

Thank you 😄

@12rambau
Copy link
Contributor Author

let's wait for the test completion but normally I'm good to go

@mariobuikhuizen
Copy link
Collaborator

One of the advantages of keeping changes as concise as possible is less chance of conflicts. #272 also changes code formatting: 32273a0#diff-85933aa74a2d66c3e4dcdf7a9ad8397f5a7971080d34ef1108296a7c6b69e7e3R25 , which leads to a conflict here.

Also, I see now that that PR removes master_doc = "index", this is certainly something different than removing comments.

@12rambau
Copy link
Contributor Author

conflict resolved. master-doc is an ignored parameter if there is an "index.rst" next to conf.py. I didn't even realized I removed it.

@mariobuikhuizen
Copy link
Collaborator

If I check the build on: https://ipyvuetify--238.org.readthedocs.build/en/238/ I don't see the new theme applied

Copy link
Collaborator

@mariobuikhuizen mariobuikhuizen left a comment

Choose a reason for hiding this comment

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

Much cleaner diff now! 😄

@mariobuikhuizen mariobuikhuizen merged commit eadb526 into widgetti:master Aug 25, 2023
@mariobuikhuizen
Copy link
Collaborator

Thanks!

@12rambau 12rambau deleted the doc branch August 25, 2023 10:47
@mariobuikhuizen
Copy link
Collaborator

mariobuikhuizen commented Aug 25, 2023

In case you think this should not be put in production yet (to which I agree) you should publish a stable version of the docs in RDT and link it in the readme. I think you need to make a GitHub release for that, else you can manually trigger the rebuild of stable. Let me know what you prefer.

Turns out older branches don't build anymore, so we're forced to put it in production. I trust it runs well.

@12rambau
Copy link
Contributor Author

ok I'll try to be as fast as possible to make the follow-up PR to have a more readable documentation ASAP

@mariobuikhuizen
Copy link
Collaborator

What less readable now?

@mariobuikhuizen
Copy link
Collaborator

Come to think of it I can also point the stable branch to just before the theme change commit. That will give more time to make your changes.

@mariobuikhuizen
Copy link
Collaborator

Production docs are back to the old theme.

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.

3 participants