Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

DOC [PST] install page #28336

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

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Feb 1, 2024

Please note that this PR targets the new_web_theme branch!

Towards #28084. This main task of this PR is to improve the installation grid, with the help of sphinx-design tabs. Please note: the primary sidebar should not have shown up. It is a known bug pydata/pydata-sphinx-theme#1662 and I have proposed a solution upstream pydata/pydata-sphinx-theme#1682.

Copy link

github-actions bot commented Feb 1, 2024

✔️ Linting Passed

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

Generated for commit: 54489fd. Link to the linter CI: here

@Charlie-XIAO

This comment was marked as outdated.

@Charlie-XIAO Charlie-XIAO marked this pull request as ready for review February 1, 2024 13:46
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Again, I like the way it looks a lot, but no idea on the js side.

@Charlie-XIAO
Copy link
Contributor Author

I think I may want to postpone this PR - I've found that sphinx-design can achieve something as nice by only writing rst stuff with minimal tweak of CSS (no complicated HTML/CSS/JS, and thus works even when JS is disabled). You can preview what it looks like in the video below, and here is the diff: new_web_theme..d6a8deb

However I'm kind of having some trouble updating dependencies because of the pandas warning in the new version, and that's why I'm showing the diff in this way. I've thought about merging main into new_web_theme and then update the dependencies... but I'm thinking that git is not able to merge the lock files correctly? So I asked in #28084 (comment) to see if the dependencies can be directly put in main.

So after the dependency problem is solved, and if maintainers like what is shown in the video below, I can just revert the reverting commit d6a8deb and this PR can be continued.

What do you think about this @adrinjalali?

Installing.scikit-learn.scikit-learn.1.5.dev0.documentation.8.-.-.Microsoft.Edge.2024-02-07.13-15-02.mp4

@Charlie-XIAO Charlie-XIAO changed the title DOC switch to pydata-sphinx-theme [3>1] install page DOC [PST] install page Feb 7, 2024
@adrinjalali
Copy link
Member

I like the idea, this looks quite good too, and better than having custom js for it.

Re dependencies, @lesteve would know better.

@lesteve
Copy link
Member

lesteve commented Feb 7, 2024

You can update your dependencies in your branch, no need to add them in main.

Add your dependencies to the doc build in:

"name": "doc",
"type": "conda",
"tag": "main-ci",
"folder": "build_tools/circle",
"platform": "linux-64",
"channel": "conda-forge",
"conda_dependencies": common_dependencies_without_coverage + [
"scikit-image",
"seaborn",
"memory_profiler",
"compilers",
"sphinx",
"sphinx-gallery",
"sphinx-copybutton",
"numpydoc",
"sphinx-prompt",
"plotly",
"polars",
"pooch",
"sphinxext-opengraph",
],
"pip_dependencies": ["jupyterlite-sphinx", "jupyterlite-pyodide-kernel"],
"package_constraints": {
"python": "3.9",
},

Add dependencies in the conda section or in the pip section depending where they are available.

Update the dependencies only for your doc build (faster):

python build_tools/update_environments_and_lock_files.py --select-build 'doc$'

There may be conflicts in the lock-files but you can sort them like any other conflicts.

@Charlie-XIAO
Copy link
Contributor Author

@lesteve Thanks for you reply! As I replied in #28084 (comment), if we do not worry about the conflicts then merging main into new_web_theme would indeed solve my problem.

@Charlie-XIAO
Copy link
Contributor Author

Why using sphinx-design?

Comparing sphinx-design tabs with my first approach (new_web_theme..54753ce):

  • Simplicity: With sphinx-design tabs we can write pure rst for the actual instructions. Either with my previous implementation or with the current scikit-learn implementation we have to write HTML. Also with sphinx-design there is no need to maintain the complicated part of either CSS or JS; only minor tweaks are needed if necessary.
  • When JS is disabled: sphinx-design tabs still work because they are CSS-based. My previous implementation would fail to work properly with JS disabled.
  • Tab sync: sphinx-design tabs support syncing, which means that if we have another set of OS-dependent instructions on this same page, switching an OS here will automatically switch the OS there. This is not currently used by maybe useful. My previous implementation does not support such usage.
  • Automatic OS selection: sphinx-design does support specifying a specific tab to select by default, by in our case the default OS selection have to be determined dynamically. My previous implementation uses JS so this can be achieved.

Why tweaking sphinx-design tabs?

This is in fact personal aesthetics choice. You may take a look at what the original nested tabs of sphinx-design look like in their documentation. I think left-aligned tabs are good if it is not nested. In the nested case the two levels of tabs are of different widths which kind of look weird from my perspective. Also I removed the box shadow at the bottom of the tab contents. Again, I think it is fine for non-nested cases, but we have nested tab sets which leads to double bottom box shadow.

What are the other changes?

There are some changes of wording in the installation instructions, so it would be nice if maintainers can take a look at the instructions under each tab to confirm their correctness. Then some indentation issues that I missed in #28107. Then about sphinx-prompt: there is now support for default modifiers for bash, batch, and powershell. I'm preferring powershell because I think nowadays Windows users use it more than cmd; also it is easier to add comments (powershell uses # same as bash, but in cmd I think one needs to use something like & REM).

@Charlie-XIAO
Copy link
Contributor Author

So the dependency problem is fixed, sphinx-design is added, and I can go with the sphinx-design tabs approach. Some explanations of the new changes: #28336 (comment). Check the rendered docs here.

@glemaitre @thomasjpfan who also commented in the first PR: would you like to take a look when you have time? 😁

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan merged commit d53cfbc into scikit-learn:new_web_theme Feb 9, 2024
@Charlie-XIAO Charlie-XIAO deleted the pydata-3_1-install branch February 9, 2024 14:58
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.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.