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

DOC final cleanup for switching to pydata-sphinx-theme #29037

Merged
merged 22 commits into from
May 20, 2024

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented May 17, 2024

Note that this PR targets the new_web_theme branch!

Towards #28084.

  • Fix merge conflicts
  • Reactivate doc-min-dependency workflow
  • Bump minimum versions of some doc dependencies
  • Gallery index page links not tweaked (don't think this is needed)
  • Some images are going wrong (cannot be found) (no issues when make html)
  • Some API examples are going wrong (looks like my local issues)
  • (Maybe) wait for pydata-sphinx-theme 0.15.3 (prerelease is already there so maybe the release will come soon?) (no need to do so, after all the website will automatically be updated when that release comes to conda-forge)

The places marked with TODO(new_web_theme) are to be resolved in #29038 after this one is merged.

  • Add how to update API documentation in development docs.

Copy link

github-actions bot commented May 17, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 1d19449. Link to the linter CI: here

@Charlie-XIAO
Copy link
Contributor Author

@glemaitre @adrinjalali I think this is ready for review. I briefly went through the website and did not see fundamental problems. After the new web theme is deployed, I may:

Please feel free to navigate through the built artifact and let me know if there are problems that I overlooked.

@Charlie-XIAO Charlie-XIAO marked this pull request as ready for review May 20, 2024 07:10
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LTGM

@glemaitre
Copy link
Member

I'm fine that we have a couple of glitches that will be removed soonish.
Fixing upstream is more important because it will impact more people. That's nice.

@glemaitre
Copy link
Member

glemaitre commented May 20, 2024

FYI, we should not block the merge of this PR regarding this #29037 (comment)

This can be tackled in a subsequent PR and I think that this is more valuable to have the website ready for 1.5

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented May 20, 2024

Sure (I'm sorry I wasn't at home so did not provide a timely update). Then shall we merge this one rn then I can quickly update #29038 to get the website ready?

@glemaitre
Copy link
Member

Don't worry, I'll ping a second reviewer but I'm sure that we will be able to do it for 1.5 :)
ping @jeremiedbb or @ogrisel

@glemaitre
Copy link
Member

I just realized that I forgot "not" in my previous sentence.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

The changes here look fine, but I haven't been following this closely so I can't tell if it's missing something

@glemaitre glemaitre merged commit aa458ac into scikit-learn:new_web_theme May 20, 2024
24 checks passed
@StefanieSenger
Copy link
Contributor

There might be something wrong here: has this PR deleted doc/modules/classes.rst by accident or on purpose?

@jeremiedbb
Copy link
Member

It was done on purpose. It's now listed in doc/api_reference.py

@glemaitre
Copy link
Member

glemaitre commented May 21, 2024

There might be something wrong here: has this PR deleted doc/modules/classes.rst by accident or on purpose?

Nop, now this file is generated with the doc/api_reference.py file. We will add an entry in the documentation to explain it.

@Charlie-XIAO Charlie-XIAO deleted the new_web_theme branch May 21, 2024 12:51
@StefanieSenger
Copy link
Contributor

Oh, I see then. Sorry, it seems I was looking for it to show up in some diff in the wrong place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants