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

DOC: start using new bootstrap-based sphinx theme #28623

Merged

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Sep 25, 2019

closes #15556

I want to propose that we start using the new bootstrap-based theme that is being developed at https://github.com/pandas-dev/pandas-sphinx-theme/ for the dev docs.
This way it will already be used for the docs at https://dev.pandas.io/docs/ so it can get some exposure before the next release.

How I did it in this PR is to install it from git master https://github.com/pandas-dev/pandas-sphinx-theme/ instead of moving the actual source into the pandas repo. I would prefer doing it like this for now, as that makes it easier to further iterate on the theme (the other repo is set up with a faster doc build (disabled ipython directive + smaller api) and automatic preview using doctr on travis). In a later stage, we can still move it here if we want (or move the pandas-specific customizations here).

A preview (of a subset, not all API pages) can be seen at https://dev.pandas.io/pandas-sphinx-theme/

There are still several "must todo's" to get the theme in a decent enough state for a release. There are some open issues on the theme repo, but I can also open an issue here to keep track of those.
And of course, feedback on the theme is very welcome.

I made two other changes:

  • moved the install.rst into the getting_started directory (so it is not a top-level navigation item). Given that the new website will have a page with a quick install linking to this more advanced install page, I think that is fine (and also, on a reworked home page of the docs it could also get a prominent place without being in the navigation bar)
  • Idem for the ecosystem page, as this will also get more exposure on the new markdown website (we actually need to remove that page / reconcile it with the website, but let's leave that for another PR)

@datapythonista
Copy link
Member

I'm ok with this if it makes your life easier to continue to work on the theme. But my preference would be to integrate the web and the docs first.

Couple of questions:

@jorisvandenbossche
Copy link
Member Author

Have you seen the missing links in https://dev.pandas.io/pandas-sphinx-theme/? Are they expected?

If you mean part of the links in the table of content in the API section, they yes, that is expected (as mentioned above, the version of the pandas docs in that repo does not build the full reference to speed up the build).

Is your plan to keep the theme as a separate project permanently, or just while finishing it?

It is my goal to move the general bootstrap stuff upstream / keep in a separate project, but there will be quite some pandas specific things (probably part of the layouts and css) that does not fit there. So after splitting it, that part could be moved to the pandas repo (or, if there would be people that want to re-use our theme, we could also keep it in the separate repo, something to further see).

@jorisvandenbossche
Copy link
Member Author

About integrating the web and docs first, I would prefer to not let this be blocked by the other issue but already merge because 1) the quicker it is merged, the quicker we can get feedback 2) it somewhat blocks Stijn to do PRs to the pandas repo for the getting started content (for the new pages we ideally can already use the bootstrap theme) and 3) it's the status quo anyhow since now there is also no clear link with the current theme.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

May be worth changing in this PR the link in the Install of the website to the "advanced installation instructions" that you moved here.

But happy to get this merged if you need it to continue your work on the theme. Feels like it'll make it trickier to integrate the web and the docs after this is merged, but we'll figure out how to move forward later on.

@@ -20,6 +20,8 @@
from numpydoc.docscrape import NumpyDocString
from sphinx.ext.autosummary import _import_by_name

import pandas_sphinx_theme # noqa
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? may be add a comment in the code to explain?

Copy link
Member Author

Choose a reason for hiding this comment

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

This might actually not be needed, will try out (I was assuming it is needed, because in the init of the package we have some sphinx extensions added, but probably sphinx imports the package anyway if you set it as the html theme)

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't seem needed. Removed.

@jorisvandenbossche
Copy link
Member Author

Feels like it'll make it trickier to integrate the web and the docs after this is merged, but we'll figure out how to move forward later on.

Can you clarify this? Merging this PR does not change anything about the theme, so if it is tricky to integrate both after merging, it is also the case without merging? Or do you mean because it is not physically in the repo?

@datapythonista
Copy link
Member

I was thinking that something like setting the templates directory in the web (defined in config.yml) to pandas/doc/theme/_templates would be an option to start the integration (then parsing the yaml file from Sphinx and providing it as context). If instead of having the theme here, we have it outside, that feels a bit trickier.

But as I said, no problem from my side to merge this, and we'll see later when we work on the integration whether this is true or not, and what to do.

@datapythonista
Copy link
Member

Tables don't look great yet, not sure if you realized:

Screenshot at 2019-09-26 17-40-21

@jorisvandenbossche
Copy link
Member Author

Tables don't look great yet, not sure if you realized:

Yep, I know. That's one of the things I ideally wanted to fix before starting to use it (it's certainly a "must fix" before the release).
There is something different here (or with bootstrap 4?) that all other sphinx themes I looked at don't have (the autosummary tables generate tables with a colwidth spec of 10% - 90%, and bootstrap tries to follow that too strictly), which I couldn't figure out yet

@datapythonista
Copy link
Member

I guess you saw it, but in case in your browser/device was working.

I'd say let's merge, close #15556, and would start opening issues for the pending things. It'd make your life much easier to finish this, than postponing the change.

@jorisvandenbossche
Copy link
Member Author

OK, will merge this later today.

@datapythonista
Copy link
Member

Will this be released for 0.25.2?

@jorisvandenbossche
Copy link
Member Author

It's in master, so no.

@jorisvandenbossche jorisvandenbossche merged commit 6396bc3 into pandas-dev:master Oct 4, 2019
@jorisvandenbossche jorisvandenbossche deleted the docs-new-theme branch October 4, 2019 20:12
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
- Use pandas-sphinx-theme in docs (from github master)
- move install into getting_started + remove ecosystem from top-level navbar
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
- Use pandas-sphinx-theme in docs (from github master)
- move install into getting_started + remove ecosystem from top-level navbar
bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020
- Use pandas-sphinx-theme in docs (from github master)
- move install into getting_started + remove ecosystem from top-level navbar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: more modern sphinx theme
2 participants