-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC Update web site to pydata-sphinx theme. #26809
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
Conversation
I think Edit: I realise what you mean. I think the lock files may have updated just because it's been a while since they were last run and there have been new versions of dependencies since |
This is the way... Thanks! |
I think it is a good idea to investigate this. Below a list of things that came to my mind while thinking about this change. It feels like every so often we have to make changes to the CSS or HTML of the current theme. In those PRs there is often a feeling of "the blind leading the blind". We don't have people who are experts on the topic. This means switching to a theme maintained by people who are (slightly more) expert at it, is a good way to not spend time on something that no one really knows about. The scikit-learn page has an "iconic look". Hate it or love it, but it is memorable. How much of that can we maintain? It also has a few more things that most documentation pages don't have, or where a given page only has one of the things. This makes me wonder how much work it is to customise the pydata theme, if it is possible and if we don't end up back in the situation where you need to be quite experienced with HTML/CSS in order to fix things. Will it lead to the URLs of pages changing? What do we do about the docs of older versions? A few years ago when the pydata theme started it felt a bit "unstable". With this I mean when you updated the version things would start working and stop working as well. This was a bit annoying. However in the last couple of years it seems like it has all stabilised a bit more. |
There are quite a few things which we need to fix for this to work, but I very much welcome the change. It's become a really nice theme and a familiar one to users who use other packages from the stack, which makes it easy for them to navigate the website. The homepage as is right now in this PR to me is similar enough to what we have, we just need to fix the links from the top bar, and some formatting issues. We also need to make sure we're dark theme friendly since fabulously this theme respects the browser's setting. Right now it looks like this: I also don't clearly see the search bar, and the version setting which we'd need to fix. Otherwise, I think this is great! |
@lucyleeow indeed, but as this is not relevant for this pr I have reverted where possible. However note that updating some dependencies may have led to the failing tests here. |
@betatim, thanks for your comments.
I'm working on the emulation of the homepage and for now updates are quite straightforward.
Do you mind giving me an example, so I can address your concerns?
I don't see any reason for that but I need more time to check.
If I understand correctly older versions will keep their look as they are linked somewhere from the stable one.... but I may be wrong ... I cannot find a lot of explicit documentation about the web infrastructure... |
We don't push to the older versions, so they'll stay as they are, which is fine, however, we should make sure switching between versions still works. This new theme also has its own way to handle different versions, I'm not sure if we wanna use it or if we want to have our own version changer thingy there. |
@adrinjalali , thanks for your comments
I agree with you, in particular I find the style of the sections less packed and more readable.
I'm working on that, please also note that font-awsome are included in the theme so I have removed the explicit link to github and added the icon in the navigation bar, together with some other social... some other may have their place there, but this is a detail for now.
I have added your remarks in the description as this will help me in organizing my time. |
I was thinking of "has quite a custom landing page", "has example gallery, examples are linked to from other parts of the docs with an image preview", "has user guide, examples and API docs", "custom(?) navigation bar/header thing". None of them individually are that complicated/novel, but I don't know many other docs pages that use all of them. So it is more of a vague list of "things to think about and look at" than a list of concrete shortcomings of the theme. |
ab4a36a
to
2c6870c
Compare
Dear all, the homepage is finalized (the testimonial carousel still need to be fixed, it is just matter of time). |
Thanks a lot for starting this work. It's an ambitious goal and a well integrated theme can bring a lot to the website. We recently built the website of https://skrub-data.org from the pydata sphinx theme but with a layout somewhat close to that of scikit-learn. There were some tricky bits on making sure that it was readable both in light and dark background. The files in skrub can be helpful: In particular, the files:
Also, one thing to keep in mind is that in scikit-learn we have a ton of custom CSS. Some of it is needed and must stay (eg for the landing page, or for the "details"' folding summary. Some of it will no longer be needed. Making the calls between what should stay and what should go will require iterations |
I should have clarified : the dark theme is not implemented yet (see checklist in the description). I have simplified the theme removing some javascripts and old css which were adding noise in the new scheme. |
From what I tested, the links work and the homepage looks quite alright. Links from examples to API docs and to user guide all seem to be fine from what I can tell. |
First kudos to @cmarmo on this change. I also like this theme (we used it in Making the migration and having all rendering rights will take some time since we don't have testing, and the only way to test is to navigate :). To me, it would make sense to create a new branch and target this PR to this branch. I think it would be beneficial for two reasons:
When we are happy with the results then we just merge this branch into main and voila. We should have very few conflicts since we usually don't modify anything from the website HTML/CSS when doing other DOC/code PRs. |
Just some notes regarding things to be fixed while navigating in light mode. This does not have to be necessarily fixed in this PR.
|
I like the suggestion of targetting a branch. It will let people make small PRs for things they spot instead of one person having to do everything. How do we setup that branch? Can we have a rendered version of that branch somewhere that is easy to look at? |
I like the suggestion of targetting a branch. It will let people make small PRs for things they spot instead of one person having to do everything.
I like that. I'll be able to do PRs :)
|
We only need to create a branch and push it into the repo.
When we will create a PR for the branch, we will have the rendering. We could also modify temporarily the script that pushes the doc into the doc repo to push specifically this branch whenever we merge a PR as well. We could then get the visualization at |
I created |
@cmarmo does this setup work for you? What do you want to do next? |
To be honest, I don´t see how adding a new branch change things:
That said, I guess it's not up to me to decide.
Reopen this PR versus the new branch, eventually. |
I'm marking it as ready, so you can merge at your convenience. |
@cmarmo : do you want to merge to the new_web_theme branch and then we (here an undefined pronoun, as I do not know who will do it) iterate on the PR? |
I'm going to be explicit... :) .... I'd rather continue to iterate on my branch. |
I'm going to be explicit... :) .... I'd rather continue to iterate on my branch.
No problem with me. Right now you are spurring this effort forward, so what matters is that you can work in the way that works for you.
I do worry that I might be commenting from the sideline, as I look at this PR (which I am excited about) and find things that can be improved. Such short messages from the peanut gallery can be annoying if you are the one bearing the main effort.
|
Ok. Let's leave things as they are for now then. Good point about people making PRs to the branch in your fork. (I think that is more polite than directly pushing to this branch.) So should we recommend to those who want to help: make a PR to Chiara's fork? |
A pro of using a branch in the repo here would be that each PR to it would be smaller and easier to review. And once the branch is ready we can merge it without having to look at the huge diff again. Just a thought |
Dear all, the dark theme is now acceptable. @glemaitre, a number of issues you noticed in your comment are gone, thanks to the fact that I have cleaned up the theme.
Rewriting a website means having a decent homepage and a working general structure, the simpler the better: in my experience this is not something you achieve with small modifications, with the risk of fixing something somewhere and bugging something else elsewhere. One have to be brave at first, and take the leap when the homepage is cleaner enough and there are no more major issues with the structure and the theme. I'm just asking you all to be patient and let me jump. Then you will be free to implement all the small changes you want. Also, I have checked how the version switcher works: this is when having a branch connected to the sklearn infrastructure will be really useful. I will not take care of that (going to edit the description too) and let those who have the power check the issue. |
Thanks, I will have a new look.
To be really explicit, I am really offended by these comments that do not bring anything to the discussion. |
@glemaitre , sorry if this blessed you. I really mean lterally. I am unable to check some options because I don't have rights to access to the infrastructure... |
I think this is stalled and can be taken over if there is still interest. |
@lucyleeow any chance you'd like to take over? 😁 |
I'm not very good with (and don't love) html/css... |
@lucyleeow I can try to continue this PR if maintainers still want to switch to this new theme (though I can't guarantee I'm good enough at HTML/CSS). P.S. I have tried a little bit in my fork https://github.com/Charlie-XIAO/scikit-learn/tree/doc-pydata though it is far from finalized. |
@Charlie-XIAO I think we do want to switch. The only thing I would check is that there are people who have time to review, it may be a big PR. |
In addition to what Lucy wrote: given how big a change this is in terms of how many things need changing and checking, etc I think it would be worth spending a bit of time trying to work out if there is a strategy that allows us to make smaller PRs that can be reviewed and merged independently. This means coming up with such a strategy would be my first step here. I expect it to be quite hard to come up with the strategy, so while it doesn't involve code it requires some serious amount of skill/experience. |
@betatim Can't agree more, and that's also what I've been concerned about in the past few weeks. It is especially hard to understand what each part of change in a huge PR is for (and because of this I gave up building on top of what cmarmo has achieved and started from scratch instead). As for the overall strategy, I would prefer making separate PRs in the main repo, but targeting the
As I've said, I have had a lot of changes ready, and I will definitely break them down so that the review process can be easier. My current plan (draft) is as follows:
Here I will list some of my results so far. (By the way, so far I've almost done the landing pages, installation page, and I'm working on fixing each page in the user guide.) @lucyleeow @betatim Do you recommend me to start a PR now, or wait for more voices? |
That sounds like a reasonable plan to me, but lets hear more voices. Can I recommend that we move this above plan to a new issue as it would be more suitable and for visibility? |
Sure, I will open an issue for that. |
What does this implement/fix? Explain your changes.
Dear maintainers,
this draft pull request updates the theme of the website to the pydata-sphinx theme.
This was suggested here.
Thanks for considering it.
Any other comments?
The pull request is a test to check how smooth would be the style conversion, a lot of details need to be fixed but in my opinion the result is already really nice... kudos to the pydata-sphinx theme developers!
I had to update dependencies to add the sphinx theme: my understanding was that I had to run
update_environments_and_lock_files.py
to do that in an automatic way. This ended up in updating the lock files too and I'm not sure it is needed. I was looking for a way to have the CI working ... Is this the way?Perhaps @lucyleeow would be interested in having a look and move forward together? 😇
Functionalities checklist
- [ ] old version switch (see comment)