-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC [PST] refactor API structure and improve display #28428
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 [PST] refactor API structure and improve display #28428
Conversation
Refactor API structurePreviously we have all classes/functions on the same page (i.e., 1-level). The problem is that in Script for generating API pages (optional)I created a script for generating API pages (contents in All-in-one templateI replaced all templates with just one Tweaking in-page tocIf you look at a class reference page, you will see that there are no entries for its methods in the secondary sidebar. This is not because they are not there, but because there parent div is not visible (if you inspect the element you will see). As second-level entries, the methods should have shown up when you scroll there, but because of this bootstrap bug To explain a bit more what this extension does, it first tells from There was a similar issue in Search on API index page (new)Proposed in #28428 (comment), I used DataTables to realize a version of API searching (by names only). The initialization script is in |
I agree the sidebars on the example in your link looks odd, so would be nice to fix that. However: personally, the API page is pretty much the only page I always have open from our website, which allows me to very quickly do a Ctrl+F and find something and with one single click I'm in the docs of that class/function. The issue with the new layout is that there's no single page where everything is listed, and this page becomes somewhat useless maybe? One would need to do a search, wait for a page reload, then find the right link, and click. I personally don't know where each class is, especially with a lot of our preprocessing stuff. |
I didn't notice that I also used it this way until you've said it 😆 Do you think it is a reasonable solution that: we add a search box on this page which only search for class/function names and display the results? This way we will not need to go to the search page, and the search speed will be significantly faster than searching through the whole documentation. |
I think anything requiring a page reload would be bad, but if you find a way to do that w/o a page reload, I'd be game. |
@adrinjalali I used DataTables to achieve something like the following. Do you think this satisfies your need? API.Reference.scikit-learn.1.5.dev0.documentation.9.-.-.Microsoft.Edge.2024-02-17.17-18-49.mp4 |
Yeah that looks pretty cool! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still kinda miss the one page listing all public API though, it'd be nice to have that. I'm not sure if that's me being used to that page or if it's really good to have it though.
Thanks @Charlie-XIAO
I think I agree: I think it's was better to get a high level overview of the library. On the other hand, the search filtering tool for the table introduced in this PR is very snappy and nice to use. Maybe we could get the best of both worlds by:
WDYT? |
Also, since the API table/search index page does not need a right-hand side navigation panel, maybe we could use that space to expand the table horizontally. |
Sorry for taking quite a while since the last update. The core of the new update is the About module name display. As suggested I used smaller font size and lower contrast color for the module names, placed under the short summaries. Other alternatives that I considered:
About default per-page entries. As suggested I showed all entries by default, but one major concern is the increased delay of initializing the table and searching. You may want to try and see if the delay is acceptable. Other comments. Since there is no deprecated API at this moment, I intentionally moved some just in order to check the rendering. When finalizing this PR they should be moved back to their right places. @ogrisel @adrinjalali Please take a look when you have time :) |
I think this is better now, yes! |
I played with the preview and it looks great to me. Will try to find the time to do a proper review. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the following is addressed:
Done, moved to under |
I'm going to self-merge since the PR has already got 2 approvals :) |
Please note that this PR targets the
new_web_theme
branch!Towards #28084.
Here is the list of things left to be done in this PR (if any). Maintainers please feel free to modify.