Skip to content
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

[MRG+1] Updates entire website design #14849

Merged
merged 155 commits into from Sep 21, 2019

Conversation

@thomasjpfan
Copy link
Member

thomasjpfan commented Aug 30, 2019

Here is a hosted version that follows this PR.

This PR:

  1. Updates the entire design of the website.
  2. Keeps all features from the original website.
  3. Adds custom sidebar with button on the lower left. (Mobile friendly)
  4. Completely custom sphinx theme that does not inherit from basic. There were a bunch of js/css we did not use, this is no longer loaded.
  5. There are no images in the new theme.
  6. Small images were generated for the logos on the bottom of the page in the index.
  7. Uses bootstrap 4.

Enjoy! :)

thomasjpfan added 13 commits Aug 29, 2019
WIP
WIP
WIP
WIP
…strap
REV
@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Aug 30, 2019

@rth

This comment has been minimized.

Copy link
Member

rth commented Aug 30, 2019

Very impressive @thomasjpfan !

Completely custom sphinx theme

I think overall this is great, though if we can afford a professional web-designer (cc @amueller) at least to review this and maybe propose some improvements on top this PR it would be ideal. Personally, I can say that the rendering looks good for me, but I really don't know to review it (with respect to cross-browser compatibility, best web-design practices etc). @cmarmo was also working on improving the website and if there is a way to collaborate on it somehow it would be great.

A side note, do we want to bundle minified jquery and bootstrap (as opposed to using a CDN)? I get they this allows to use the doc offline, but they are not that small and a prone to be updated frequently.

@rth

This comment has been minimized.

Copy link
Member

rth commented Aug 30, 2019

For instance, I also wonder if bootstrap is still the best way to go in the frontend world. Though I agree it's a clear improvement over our current website in any case.

@rth

This comment has been minimized.

Copy link
Member

rth commented Aug 30, 2019

For instance, I also wonder if bootstrap is still the best way to go in the frontend world. Though I agree it's a clear improvement over our current website in any case.

FWIW, pytorch also uses bootstrap but their theme is very customized.

thomasjpfan added 9 commits Aug 30, 2019
@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Aug 30, 2019

Personally, I can say that the rendering looks good for me, but I really don't know to review it

Not having something like webpack to do auto prefixing is a little rough. I have been developing in firefox/chrome/safari. (edge is moving to chromium) I will get my hands on IE and see what it looks like.

FWIW, pytorch also uses bootstrap but their theme is very customized.

They vendor their bootstrap as well and not use the bootstrap CDN. (It is pretty nice to view docs locally)

For instance, I also wonder if bootstrap is still the best way to go in the frontend world. Though I agree it's a clear improvement over our current website in any case.

Bootstrap is really nice to prototype design in because I was most familiar with it. Now that we have a design, I think it is possible to use something smaller, like purecss. We would only need custom js for the "who uses scikit-learn" carousel and maybe for the navbar folding in mobile. Although, using a familiar/popular css framework like Bootstrap makes it easier for others to contribute/modify.

thomasjpfan added 2 commits Aug 30, 2019
@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented Sep 18, 2019

@jnothman any further comments? merge?

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Sep 18, 2019

Copy link
Member

jnothman left a comment

Otherwise, good to merge!!

Wow, @thomasjpfan

doc/conf.py Outdated
@@ -314,8 +309,8 @@ def make_carousel_thumbs(app, exception):

def setup(app):
# to hide/show the prompt in code examples:
app.add_javascript('js/copybutton.js')
app.add_javascript('js/extra.js')
# app.add_javascript('js/copybutton.js')

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member

What's this about? I don't thnk we should have commented-out code lying around

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Sep 20, 2019

Author Member

Removed.

@@ -0,0 +1,111 @@
{% extends "layout.html" %}
{% set title = 'Documentation scikit-learn: machine learning in Python' %}

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member
Suggested change
{% set title = 'Documentation scikit-learn: machine learning in Python' %}
{% set title = 'Documentation of scikit-learn: machine learning in Python' %}
<div class="card h-100 sk-documentation-index-card mx-md-2">
<a href="{{ pathto('tutorial/basic/tutorial') }}" class="stretched-link sk-documentation-index-anchor"></a><h4 class="card-header">Quick Start</h4>
<div class="card-body p-3">
<p class="card-text">A very short introduction into machine learning problems and how to solve them using scikit-learn. Presents basic concepts and conventions.</p>

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member
Suggested change
<p class="card-text">A very short introduction into machine learning problems and how to solve them using scikit-learn. Presents basic concepts and conventions.</p>
<p class="card-text">A very short introduction to machine learning problems and how to solve them using scikit-learn. Presents basic concepts and conventions.</p>
<div class="card h-100 sk-documentation-index-card mx-md-2">
<a href="{{ pathto('modules/classes') }}" class="stretched-link sk-documentation-index-anchor"></a><h4 class="card-header">API</h4>
<div class="card-body p-3">
<p class="card-text">The exact API of all functions and classes, as given by the docstrings. The API documents expected types and allowed features for all functions, and all parameters available for the algorithms.</p>

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member
Suggested change
<p class="card-text">The exact API of all functions and classes, as given by the docstrings. The API documents expected types and allowed features for all functions, and all parameters available for the algorithms.</p>
<p class="card-text">The exact API of all functions and classes, as given by the docstrings. The API describes expected types and allowed features for all functions, and all parameters available for the algorithms.</p>

"The API documents" was a garden path sentence, since "documents" is ambiguously a noun and verb.

<div class="card h-100">
<div class="card-body">
<a href="supervised_learning.html#supervised-learning"><h4 class="sk-card-title card-title">Classification</h4></a>
<p class="card-text">Identifying to which category an object belongs to.</p>

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member
Suggested change
<p class="card-text">Identifying to which category an object belongs to.</p>
<p class="card-text">Identifying which category an object belongs to.</p>

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member

Yes, these are typos in the original.

I'd like to change these all to imperative mod: "Identify which category..." but perhaps in the next PR.

<a class="btn btn-warning btn-big sk-donate-btn mb-1" onclick="document.getElementById('paypal-form').submit(); ">Help us, <strong>donate!</strong></a>
<a class="btn btn-warning btn-big mb-1" href="about.html#citing-scikit-learn"><strong>Cite us!</strong></a>
<p class="mt-2">
<a href="about.html#funding">Read more about donations</a>

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member

(this seems to be a poor link target)

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Sep 20, 2019

Author Member

Poor as in...its too small?

});

{%- if pagename != 'index' and pagename != 'documentation' %}
/*** Had navbar when scrolling down ***/

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member

What is "Had navbar"?

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Sep 20, 2019

Author Member

It meant to "Hide"

stylesheet = css/theme.css

[options]
google_analytics = false

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member

I'm confused about this. It seems to be in contradiction to conf.py

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Sep 20, 2019

Author Member

This sets the default. I can change it to true by default.

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Sep 20, 2019

Author Member

This was changed to true.

{% endif %}

<script>
$(document).ready(function() {

This comment has been minimized.

Copy link
@jnothman

jnothman Sep 18, 2019

Member

Why is it preferably to include this in the page, rather than by src?

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Sep 20, 2019

Author Member

This reduces the number of HTTP requests.

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Sep 20, 2019

Author Member

The total number of bytes the javascript.html is small (in bytes), so I thought it was worth it to inline the javascript.

thomasjpfan and others added 5 commits Sep 20, 2019
…strap
@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Sep 21, 2019

LGTM.

Merge? @jnothman : you had comments. I'll leave you some time to voice any other comments if you have some. If I don't hear from you, I'll merge.

@jnothman jnothman merged commit 89332d3 into scikit-learn:master Sep 21, 2019
19 checks passed
19 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing b02217d...0478a28
Details
codecov/project 96.76% (+<.01%) compared to b02217d
Details
scikit-learn.scikit-learn Build #20190921.2 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda_mkl) Linux pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Sep 21, 2019

Awesome work, @thomasjpfan!

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Sep 23, 2019

Sorry I didn't follow the discussion.
If I click on "install" the sidebar has a toc of install, and similarly for examples and API. But if I click on User Guide, the sidebar has a toc for the full page that includes the API and examples. That's a bit confusing. Here the sidebar doesn't correspond to the things on the left (which kind of makes sense since you don't want to show the same thing twice), but I find it confusing.

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Sep 23, 2019

also, the "documentation" page now seems a bit useless, right?

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Sep 23, 2019

also weird line-wrap:
image

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Sep 23, 2019

Also, I would maybe remove the testimonials. They are mostly confusing if anything. Linking to the sklearn consortium or the google scholar citations would be more informative, I think.

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Sep 23, 2019

If I click on "install" the sidebar has a toc of install, and similarly for examples and API. But if I click on User Guide, the sidebar has a toc for the full page that includes the API and examples. That's a bit confusing. Here the sidebar doesn't correspond to the things on the left (which kind of makes sense since you don't want to show the same thing twice), but I find it confusing.

The sidebar changes based on context. Before the sidebar was just useless: https://scikit-learn.org/stable/user_guide.html

also, the "documentation" page now seems a bit useless, right?

It is less useful now. It still gives a little context to each link.

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Sep 23, 2019

There has been a discussion on how to structure numpy, scipy and pandas websites here:
numpy/numpy.org#43

It might make sense to join the discussion and/or follow suite.

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Sep 23, 2019

It is less useful now. It still gives a little context to each link.

well but when/how would a user end up there? like if they don't know what of the other links to click? seems like a strange path to take.

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Sep 23, 2019

well but when/how would a user end up there

They can end up there from the landing page if they click "Documentation":

Screen Shot 2019-09-23 at 4 41 24 PM

well but when/how would a user end up there? like if they don't know what of the other links to click? seems like a strange path to take.

It kind of serves as a global index. I am okay with removing it.

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Sep 23, 2019

Ah, I hadn't seen that. I'm not opposed to keeping it but it seems less useful now.

@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented Oct 2, 2019

Same feeling.

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Oct 5, 2019

One of the ways we are using the documentation page is to outline all the versions in the all versions page (It shows the version number on the top of the page)



@media screen and (min-width: 1540px) {
.sphx-glr-download-link-note {

This comment has been minimized.

Copy link
@lesteve

lesteve Oct 19, 2019

Member

Great stuff! I just noticed that the note about the binder link is floated right, I wish I had the web skills to think of this in the first place and then execute it as well.
image

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Oct 23, 2019

@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Oct 23, 2019

Should we consider backporting this to sphinx-gallery?

Sounds like a good idea indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.