Skip to content

DOC tweak appearance of installation instructions #29160

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

Merged
merged 7 commits into from
Jul 3, 2024

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Jun 2, 2024

Fixes #29152. This PR mainly:

  • Moves OS selection to the first level.
  • Makes the tabs button-like.
  • Removes the background and border of tab contents.

https://output.circle-artifacts.com/output/job/e64b46ad-9ed6-410e-8f02-35157656fc21/artifacts/0/doc/install.html

Installing.scikit-learn.scikit-learn.1.6.dev0.documentation.16.-.-.Microsoft.Edge.2024-06-03.01-36-08.mp4

Copy link

github-actions bot commented Jun 2, 2024

✔️ Linting Passed

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

Generated for commit: ce2eefe. Link to the linter CI: here

Copy link
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

Thanks this looks a lot better to me! A couple of comments.

@lesteve
Copy link
Member

lesteve commented Jun 3, 2024

Last comment, at one point, I think we had the OS automatically chosen based on the browser OS 1. This would be nice to have too ... I guess this needs some javascript to do this so maybe a different PR.

Footnotes

  1. maybe it was in another project like nilearn

@Charlie-XIAO
Copy link
Contributor Author

Sorry for the very late reply.

Maybe try to use substitution to reuse the same conda content in multiple places: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#substitutions

Not sure if that works with complex multi-line content, if not Including a file may be an option?

I don't think substitutions work for multiline, but yes .. include:: works.

"Packager" sounds a bit weird, maybe "Installer" instead, or "Install method" ? I know it was the wording used previously in scikit-learn 1.4 but still ...

Edit: I have seen that PyTorch uses "Package" which I find slightly weird too ...

Package manager

Changed "Packager" to "Package Manager" as suggested and "OS" to "Operating System" to have a similar length. Long names don't fit well into the same line on small screens so they are only displayed for screen width larger than 960px (I could have played with the flex box to make the captions displayed above the tabs on small screens but I just don't find it necessary).

Last comment, at one point, I think we had the OS automatically chosen based on the browser OS. This would be nice to have too ... I guess this needs some javascript to do this so maybe a different PR.

I did try in my initial rework of the installation page here, but here since we don't have full control of how things are rendered and sphinx-design has those synchonized tabs things etc., it would be harder to correctly override the behavior with JavaScript but I will give it a try.

@lesteve
Copy link
Member

lesteve commented Jun 21, 2024

I did try in my initial rework of the installation page here, but here since we don't have full control of how things are rendered and sphinx-design has those synchonized tabs things etc., it would be harder to correctly override the behavior with JavaScript but I will give it a try.

OK thanks, to be honest, since you have tried already and it does indeed look quite painful, then let's drop this this is not so crucial.

Copy link
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

LGTM, the rendered doc is here for a second review

Edit: I pushed a packager -> package-manager in the rst, hopefully I did not break anything.

@Charlie-XIAO
Copy link
Contributor Author

I think the failing tests were unrelated to this PR, probably due to #29100.

@lesteve
Copy link
Member

lesteve commented Jun 21, 2024

Yeah you will need to merge main when #29328 is auto-merged 😓

@lesteve lesteve merged commit 1ab5d0d into scikit-learn:main Jul 3, 2024
30 checks passed
@lesteve
Copy link
Member

lesteve commented Jul 3, 2024

In it goes, thanks a lot @Charlie-XIAO!

I wished Github has some kind of feature like "merge this in 7 days if noone else comments/complaints", so easy to forget completely fine PRs like this one ...

@Charlie-XIAO Charlie-XIAO deleted the install-grid branch July 3, 2024 07:05
@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Jul 3, 2024

Oops, looks like we forgot to change a "packager" in the css:

image

I will provide a quick fix.

@lesteve
Copy link
Member

lesteve commented Jul 3, 2024

Indeed I was looking at the wrong CircleCI artifact sorry for that 😅 ...

snath-xoc pushed a commit to snath-xoc/scikit-learn that referenced this pull request Jul 5, 2024
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 9, 2024
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
glemaitre pushed a commit that referenced this pull request Sep 11, 2024
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
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.

Slightly weird installation buttons maybe due to pydata-sphinx-theme 1.5.3?
3 participants