Use `sphinx-theme-builder` to build package #419

Eric-Arellano · 2023-06-26T22:40:32Z

sphinx-theme-builder allows us to use JavaScript/NPM in our Python packaging pipeline. Whereas right now we have to write raw CSS and JavaScript files, we can now pre-process those files.

sphinx-theme-builder automates installing Node and NPM for users. It runs npm run build to pre-compile our code, which for now simply copies our CSS with cp to keep the code simple.

sphinx-theme-builder gives some benefits:

Minify our CSS and JS, which is important for site loading performance. Minify theme assets? #283
No performance hit to splitting our CSS into multiple files
We can use Sass, which is more maintainable than CSS
We can more easily vendor and tweak Furo's JS code, which allows fixing Furo: anchor links still broken for page table of contents #368

Changes folder structure

Before:

src
└── qiskit_sphinx_theme
    ├── ecosystem
    ├── furo  # the core qiskit theme
    └── pytorch

After:

src
└── qiskit_sphinx_theme
    ├── assets  # Sass and JS for core qiskit theme
    ├── ecosystem
    ├── pytorch
    └── theme  # the core qiskit theme

Python's official [packaging guide](https://packaging.python.org/en/latest/tutorials/packaging-projects/) now recommends using the `src/` layout, rather than having a top level module folder called `qiskit_sphinx_theme`. For example, this layout works Out Of The Box with setuptools—we no longer need to explicitly set `packages` in `setup.py`. This change unblocks us from migrating to qiskit-sphinx-theme in #419 because it expects projects to use the standard `src` layout. This has no impact on end-users. It only impacts how we layout our code. When running `python3 -m build`, the final result will strip out the `src/`. ## Also renames top-level folders Before: ``` qiskit_sphinx_theme ├── __init__.py ├── furo │ ├── base # the core qiskit theme │ └── ecosystem └── pytorch ``` After: ``` src └── qiskit_sphinx_theme ├── ecosystem ├── furo # the core qiskit theme └── pytorch ``` When I first set up the folders two months ago, I thought that we would have Theme Variants with Pytorch. I also didn't realize that the Ecosystem Furo theme variant will be so heavily based on the core Furo Qiskit theme; that is, we won't have `furo/base`, `furo/qiskit`, and `furo/ecosystem`. We'll simply have something like `qiskit/` and `ecosystem/`, with the Ecosystem theme based directly off of the Qiskit theme.

`pyproject.toml` is now the standardized way to set packaging metadata for Python packages. When possible, setuptools recommends using it rather than a dynamic `setup.py` time. Switching to `pyproject.toml` specifically unblocks #419. Another benefit is we can deduplicate setting the version in two places. ## Gets rid of "universal" wheel We were incorrectly claiming in `setup.cfg` that the package is "universal", which means it works with both Python 2 and Python 3. That is a lie. We don't support Python 2.

Eric-Arellano · 2023-06-28T00:57:37Z

.github/workflows/main.yml

+      - name: Fix permissions for sphinx-theme-builder
+        run: chown -R $(id -u):$(id -g) .


ChatGPT 🙇

package.json

Eric-Arellano · 2023-06-28T00:59:12Z

src/qiskit_sphinx_theme/__init__.py

+def activate_themes(app: sphinx.application.Sphinx, config: sphinx.config.Config) -> None:
+    if config.html_theme == "_qiskit_furo":


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!

Eric-Arellano · 2023-06-28T00:59:43Z

src/qiskit_sphinx_theme/__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)


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

Eric-Arellano · 2023-06-28T01:00:13Z

src/qiskit_sphinx_theme/theme/qiskit-sphinx-theme/theme.conf

-# Our stylesheet takes precedence.
-stylesheet =
-  styles/furo.css,
-  styles/furo-extensions.css,


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

frankharkins

Looks good! The Furo theme works correctly for me, and I didn't notice any problems with the diff. Will obviously need review from someone with more sphinx experience than me though.

src/qiskit_sphinx_theme/__init__.py

javabster · 2023-06-28T13:14:26Z

CONTRIBUTING.md

@@ -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`


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?

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

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?

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

javabster · 2023-06-28T13:24:35Z

CONTRIBUTING.md


 ### 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.


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

Yes. sphinx-theme-builder is extremely opinionated

…inx-theme-builder

javabster

LGTM, just left one more question about the folder structure 😅

Closes #496. #419 accidentally broke Pytorch because it apparently doesn't work to call `app.setup_extension` inside the `config-inited` event. So, we were no longer activating jQuery by default.

Closes Qiskit#496. Qiskit#419 accidentally broke Pytorch because it apparently doesn't work to call `app.setup_extension` inside the `config-inited` event. So, we were no longer activating jQuery by default.

Closes #496. #419 accidentally broke Pytorch because it apparently doesn't work to call `app.setup_extension` inside the `config-inited` event. So, we were no longer activating jQuery by default.

Eric-Arellano mentioned this pull request Jun 26, 2023

Reorganize project to use src/ layout #420

Merged

Eric-Arellano force-pushed the sphinx-theme-builder branch from 27422eb to af7bee3 Compare June 26, 2023 23:27

Eric-Arellano mentioned this pull request Jun 27, 2023

[wip] Split up Furo CSS into 3 files #416

Closed

Eric-Arellano mentioned this pull request Jun 27, 2023

Migrate from setup.py to pyproject.toml #422

Merged

Set up sphinx-theme-builder

9b1160c

Eric-Arellano force-pushed the sphinx-theme-builder branch from af7bee3 to 9b1160c Compare June 27, 2023 23:31

Eric-Arellano added 4 commits June 27, 2023 18:23

Fix conditional loading of themes

a72a9fc

Move CSS to correct location

84b889f

Update contributing guide

5bbbe8f

Try out recommendation for permissions

379dc84

Eric-Arellano changed the title ~~[proof of concept] Use sphinx-theme-builder to build package~~ Use sphinx-theme-builder to build package Jun 28, 2023

Eric-Arellano marked this pull request as ready for review June 28, 2023 00:57

Eric-Arellano commented Jun 28, 2023

View reviewed changes

Eric-Arellano added 2 commits June 27, 2023 19:03

Improve wording

ededa76

Fix npm build

bfeef13

Eric-Arellano force-pushed the sphinx-theme-builder branch from 79cd1bf to bfeef13 Compare June 28, 2023 01:17

Eric-Arellano requested review from javabster and coruscating June 28, 2023 01:23

This comment was marked as outdated.

Sign in to view

frankharkins approved these changes Jun 28, 2023

View reviewed changes

frankharkins reviewed Jun 28, 2023

View reviewed changes

src/qiskit_sphinx_theme/__init__.py Show resolved Hide resolved

javabster reviewed Jun 28, 2023

View reviewed changes

Merge branch 'main' of github.com:Qiskit/qiskit_sphinx_theme into sph…

c213694

…inx-theme-builder

javabster approved these changes Jun 28, 2023

View reviewed changes

Eric-Arellano merged commit 2a646ec into Qiskit:main Jun 28, 2023
4 checks passed

Eric-Arellano deleted the sphinx-theme-builder branch June 28, 2023 17:02

This was referenced Jul 17, 2023

Pytorch's translations feature broke in 1.13 #496

Closed

Fix Pytorch theme not activating jQuery #497

Merged

Eric-Arellano mentioned this pull request Jul 17, 2023

Fix Pytorch theme not activating jQuery (Cherry-pick of #497) #503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `sphinx-theme-builder` to build package #419

Use `sphinx-theme-builder` to build package #419

Eric-Arellano commented Jun 26, 2023 •

edited

Loading

Eric-Arellano Jun 28, 2023

Eric-Arellano Jun 28, 2023

Eric-Arellano Jun 28, 2023

Eric-Arellano Jun 28, 2023

This comment was marked as outdated.

frankharkins left a comment

javabster Jun 28, 2023

Eric-Arellano Jun 28, 2023

javabster Jun 28, 2023

Eric-Arellano Jun 28, 2023

javabster Jun 28, 2023

Eric-Arellano Jun 28, 2023

javabster left a comment

		- name: Fix permissions for sphinx-theme-builder
		run: chown -R $(id -u):$(id -g) .

		def activate_themes(app: sphinx.application.Sphinx, config: sphinx.config.Config) -> None:
		if config.html_theme == "_qiskit_furo":

Use sphinx-theme-builder to build package #419

Use sphinx-theme-builder to build package #419

Conversation

Eric-Arellano commented Jun 26, 2023 • edited Loading

Changes folder structure

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

frankharkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javabster left a comment

Choose a reason for hiding this comment

Use `sphinx-theme-builder` to build package #419

Use `sphinx-theme-builder` to build package #419

Eric-Arellano commented Jun 26, 2023 •

edited

Loading