-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC [PST] landing page #28331
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] landing page #28331
Conversation
Why generating
|
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.
No opinion on the js
part, but I like the rendered version, neat. And the JS is quite short.
@glemaitre: Do you have time for a review? 😁 |
I'm not a great fan of using JS to do the alignment. The main reason being that the content renders unaligned, I start looking at it and then a second or so later it gets moved. I admit it is a pet peeve born out of trying to interact with a UI element just in the moment it moves (mostly on my phone). Which often results in me clicking something else, having to go back, etc, etc I had a look through the boostrap (v5) documentation to see if they offer a solution or advice/opinion on this topic but couldn't find anything. Currently on scikit-learn.org/ there is no alignment. So overall I'd vote for not doing alignment with JS. |
Thanks for the catch @betatim, I didn't really notice this problem. I've just solved a similar issue in |
This looks ready to merge to me or is there something else that needs doing? |
There's nothing else to do on my side. Do we need more maintainers to approve? |
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.
A few comments and a question:
is it intentional that the top level navbar is now fixed when scrolling on the landing page?
It used to be the case that this would only happen on non-home pages prior to this PR.
I have no strong opinion either way. At least now the behavior is more consistent across pages. I am not sure if the special casing of the home page navbar was intentional or not. Maybe @thomasjpfan knows?
I believe in the current theme the behavior is intentional: there is |
It was intentional, but I am okay with this PR which makes it consistent. |
Alright, I am fine with the consistently fixed behavior of the navbar on all pages including the landing page. Is there some old navbar related HTML/CSS to clean-up as a result? The Similarly, the |
Everything under the original themes folder is unused now. I didn't remove them only to be able to search things more easily. Theoretically the whole folder can be deleted at any time. The new CSS (under the scss folder) should contain no redundant code because I took only relevant parts in the first place instead of copy-paste then remove. |
Time to merge then? Cleaning up the old theme folder could be a new PR? |
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.
Minor suggestion that would also apply for other .scss files but otherwise, LGTM.
Note: while building this branch with
This is probably unrelated to this PR as the |
Thanks for spotting that. I will look into it and fix with another PR. |
I added the comment for all current |
Merged! thanks again for the PR. |
Please note that this PR targets the
new_web_theme
branch!Towards #28084. This main task of this PR is to retain the iconic look of the scikit-learn landing page, while adapting to the dark theme at our best effort.