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

Use sphinx-theme-builder to build package #419

Merged
merged 8 commits into from
Jun 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ jobs:
- uses: actions/checkout@v3
with:
fetch-depth: 0
- name: Fix permissions for sphinx-theme-builder
run: chown -R $(id -u):$(id -g) .
Comment on lines +20 to +21
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ChatGPT 🙇

- name: Set up Node.js
uses: actions/setup-node@v3
with:
Expand Down
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,8 @@ dmypy.json
# JavaScript
/node_modules
/snapshot_results
/.nodeenv/

# These get copied from sphinx-theme-builder.
/src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/static/styles
/src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/static/scripts
20 changes: 12 additions & 8 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@ contributing to qiskit_sphinx_theme, these are documented below.
There are a few important subfolders to be aware of:

### `/src`
This subfolder contains the raw HTML, CSS, and Python files that are used to set the framework for Qiskit documentation web pages. The files in this folder will set the styles when a project calls the `qiskit_sphinx_theme` in its `conf.py`, unless the project overrides the style with their own files.
This subfolder contains the HTML, CSS, and Python files that are used for the Qiskit themes. It has these folders:

E.g. if a qiskit project (with the `qiskit_sphinx_theme` installed) wants to override any of the themes files (such as the `layout.html`) that the `qiskit_sphinx_theme` provides it needs to have its own version of that file (e.g. `layout.html`) stored in its `docs/_templates` folder.
* `assets`: CSS and JavaScript for our Furo theme, which is the future of the Sphinx theme.
* `pytorch`: all the code for the legacy Pytorch theme.
* `theme`: static files, like HTML templates, for our Furo theme.

The top-level Python files are used for logic used by the theme, such as `translations.py` determining what URLs the HTML should use for translations support.

### `/example_docs`
This subfolder contains a scaled down version of the Sphinx build that builds the documentation for the Qiskit repos.
Expand Down Expand Up @@ -102,13 +106,13 @@ To update the top nav bar web component:
1. In https://github.com/Qiskit/web-components, run `npm install` then `npm run build`.
2. There should be a file created at the root of the web components repository called `experimental-bundled-ui-shell.js`. Copy its contents into these files in this theme repository:
1. `src/qiskit_sphinx_theme/pytorch/static/js/web-components/top-nav-bar.js`
2. `src/qiskit_sphinx_theme/furo/static/js/web-components/top-nav-bar.js`
2. `src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/static/js/web-components/top-nav-bar.js`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a general q about the new folder structure. src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/etc feels very long and repetitive, do we really need so many subfolders? Could it be simplified to src/qiskit_sphinx_theme/theme/etc or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

100% agreed. But yes, sphinx-theme-builder is extremely opinionated

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm that's annoying 🤔 would it be an option to remove a folder layer higher up? So have something like this:

src
 ├── assets 
 ├── ecosystem
 ├── pytorch
 └── theme 
      └── qiskit-sphinx-theme

or would that mess things up as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would break how Python packages work. We could no longer import qiskit_sphinx_theme. And I think sphinx-theme-builder would complain also too hah

3. Build the example docs with `tox -e docs` and `THEME=_qiskit_furo tox -e docs` to ensure everything works.

If you want to add a new web component:

1. Work with the web components repository's team to set up `npm run build` to generate a single JavaScript file.
2. Add the file contents to the `src/qiskit_sphinx_theme/furo/static/js/web-components/` folder in this theme repository. (We shouldn't add web components to Pytorch.)
2. Add the file contents to the `src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/static/js/web-components/` folder in this theme repository. (We shouldn't add web components to Pytorch.)
3. Load the web component in `extra_head.html` with a line like `<script src="{{ pathto('_static/js/web-components/my-component.js', 1) }}"></script>`.
4. Use the web component element in the relevant HTML, e.g. `<my-component>` in `layout.html`. Remember to surround the change with a `QISKIT CHANGE:` comment.
5. Build the example docs with `THEME=_qiskit_furo tox -e docs` to ensure everything works.
Expand All @@ -129,7 +133,7 @@ Contributors with write access can also use live previews of the docs: GitHub wi
------
## FYI: How Furo Theme Inheritance Works

We use Sphinx's inheritance future for our Furo theme, which we set in `furo/theme.conf`. Sphinx will default to using all the files from Furo. But if we have a file with the same name as Furo, then Sphinx will use our copy. That allows us to override only what we care about.
We use Sphinx's inheritance future for our Furo theme, which we set in `theme/qiskit-sphinx-theme/theme.conf`. Sphinx will default to using all the files from Furo. But if we have a file with the same name as Furo, then Sphinx will use our copy. That allows us to override only what we care about.

We try to keep changes to a minimum because every divergence we make from base Furo increases our maintenance burden. Hence we prioritise only making changes that are important to the Qiskit brand. If the change would be generally useful to other users of Furo, we try to contribute upstream to the Furo project itself.

Expand All @@ -149,10 +153,10 @@ When making changes, use those comments to make clear where and what we changed.
{#- QISKIT CHANGE: end. -#}
```

If the change is greater than 1-3 lines, write the code in a new file in `custom_templates`, then use Jinja's `include` directive, as shown in the example right above.
If the change is greater than 1-3 lines, write the code in a new file in `theme/qiskit-sphinx-theme/custom_templates`, then use Jinja's `include` directive, as shown in the example right above.

### How to change CSS
Make CSS changes in the file `qiskit_changes.css`. It takes precedence over any CSS rules from Furo.
Make CSS changes in the file `assets/styles/qiskit-sphinx-theme.css`. It takes precedence over any CSS rules from Furo.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it a requirement that the css file is called qiskit-sphinx-theme? I quite liked the qiskit_custom.css bc it easily implied that this was where stuff differed from base Furo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. sphinx-theme-builder is extremely opinionated


When adding changes, document the rationale unless the code is already self-documenting and obvious. Group similar changes into sections.

Expand All @@ -164,7 +168,7 @@ Update the version in `pyproject.toml`. Always pin to an exact version of Furo.
However, when updating, closely analyze each commit in the release to check for any changes that would break our fork. We want to make sure that our HTML files are always in sync with Furo. If they have made any changes, then add them back to our copy of the file.

### How to add an icon
Edit the file `furo/partials/icons.html`. Copy the HTML code of the `<svg></svg>` tags and add them as the first element within the `<symbol>` tag. Don't forget to include the `id` attribute, which will serve as the name associated with the icon.
Edit the file `theme/qiskit-sphinx-theme/partials/icons.html`. Copy the HTML code of the `<svg></svg>` tags and add them as the first element within the `<symbol>` tag. Don't forget to include the `id` attribute, which will serve as the name associated with the icon.

To use the icon, reference it with `#`.

Expand Down
4 changes: 0 additions & 4 deletions MANIFEST.in

This file was deleted.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"playwright": "^1.34.3"
},
"scripts": {
"build": "mkdir src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/static/styles; cp src/qiskit_sphinx_theme/assets/styles/qiskit-sphinx-theme.css src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/static/styles/qiskit-sphinx-theme.css",
"start": "http-server example_docs/docs/_build/html",
"test-snapshots": "npm run _docker-build && npm run _docker-run",
"_docker-build": "docker build -t qiskit_sphinx_theme -f tests/js/Dockerfile .",
Expand Down
7 changes: 5 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[build-system]
requires = ["setuptools"]
build-backend = "setuptools.build_meta"
requires = ["sphinx-theme-builder>=0.2.0b2"]
build-backend = "sphinx_theme_builder"

[project]
name = "qiskit-sphinx-theme"
Expand Down Expand Up @@ -40,6 +40,9 @@ _qiskit_furo = "qiskit_sphinx_theme"
"Bug Tracker" = "https://github.com/Qiskit/qiskit_sphinx_theme/issues"
"Source Code" = "https://github.com/Qiskit/qiskit_sphinx_theme"

[tool.sphinx-theme-builder]
node-version = "18.16.0"


[tool.pytest.ini_options]
addopts = ["--import-mode=importlib"]
Expand Down
35 changes: 14 additions & 21 deletions src/qiskit_sphinx_theme/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,12 @@
from pathlib import Path
from typing import TYPE_CHECKING

import sphinx.addnodes

from qiskit_sphinx_theme import directives, previous_releases, translations

if TYPE_CHECKING:
import sphinx.addnodes
import sphinx.application
import sphinx.config

__version__ = "1.12.0rc1"
__version_full__ = __version__
Expand Down Expand Up @@ -68,6 +67,17 @@ def remove_thebe_if_not_needed(
]


def activate_themes(app: sphinx.application.Sphinx, config: sphinx.config.Config) -> None:
if config.html_theme == "_qiskit_furo":
Comment on lines +70 to +71
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, apparently, the old code I had didn't actually work. Oops. The config.html_theme was always alabaster because it was being evaluated too early.

This works!

# We set a low priority so that our Qiskit CSS file overrides Furo.
app.add_css_file("styles/furo.css", 100)
app.add_js_file("scripts/furo.js")
else:
# Sphinx 6 stopped including jQuery by default. Our Pytorch theme depend on jQuery,
# so activate it for our users automatically.
app.setup_extension("sphinxcontrib.jquery")
frankharkins marked this conversation as resolved.
Show resolved Hide resolved


# See https://www.sphinx-doc.org/en/master/development/theming.html
def setup(app: sphinx.application.Sphinx) -> dict[str, bool]:
# Used to generate URL references. Expected to be e.g. `ecosystem/finance`.
Expand All @@ -82,26 +92,9 @@ def setup(app: sphinx.application.Sphinx) -> dict[str, bool]:
translations.setup(app)

app.add_html_theme("qiskit_sphinx_theme", _get_theme_absolute_path("pytorch"))
app.add_html_theme("_qiskit_furo", _get_theme_absolute_path("furo"))
app.add_html_theme("_qiskit_furo", _get_theme_absolute_path("theme/qiskit-sphinx-theme"))

app.connect("html-page-context", remove_thebe_if_not_needed)

if app.config.html_theme == "_qiskit_furo":
# The below must be kept in sync with `furo/__init__.py`.
from furo import (
WrapTableAndMathInAContainerTransform,
_builder_inited,
_html_page_context,
_overwrite_pygments_css,
)

app.add_post_transform(WrapTableAndMathInAContainerTransform)
app.connect("html-page-context", _html_page_context)
app.connect("builder-inited", _builder_inited)
app.connect("build-finished", _overwrite_pygments_css)
Comment on lines -91 to -101
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently, none of this is necessary. It gets called already because our Furo theme inherits from Furo.

else:
# Sphinx 6 stopped including jQuery by default. Our Pytorch theme depend on jQuery,
# so activate it for our users automatically.
app.setup_extension("sphinxcontrib.jquery")
app.connect("config-inited", activate_themes)

return {"parallel_read_safe": True, "parallel_write_safe": True}
4 changes: 4 additions & 0 deletions src/qiskit_sphinx_theme/assets/scripts/qiskit-sphinx-theme.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Empty file required by sphinx-theme-builder.
//
// This will allow us to fix Furo's JavaScript code to workaround
// https://github.com/Qiskit/qiskit_sphinx_theme/issues/368.
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
[theme]
inherit = furo

# Our stylesheet takes precedence.
stylesheet =
styles/furo.css,
styles/furo-extensions.css,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need to explicitly register furo-extensions.css apparently. It already gets registered by Furo, thanks to us inheriting Furo.

styles/qiskit_changes.css
stylesheet = styles/qiskit-sphinx-theme.css

sidebars =
custom_templates/sidebar_search.html,
Expand Down
6 changes: 3 additions & 3 deletions tests/py/web_components_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@


def test_top_nav_bar():
furo = Path("src/qiskit_sphinx_theme/furo/static/js/web-components/top-nav-bar.js").read_text(
"utf-8"
)
furo = Path(
"src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/static/js/web-components/top-nav-bar.js"
).read_text("utf-8")
pytorch = Path(
"src/qiskit_sphinx_theme/pytorch/static/js/web-components/top-nav-bar.js"
).read_text("utf-8")
Expand Down