MAINT: Switch to sphinx-theme-builder #469

AakashGfude · 2022-01-18T13:07:39Z

This PR switches to using sphinx-theme-builder, following this issue #417

choldgraf

A couple quick thoughts - thanks!

noxfile.py

choldgraf · 2022-01-18T19:02:12Z

package.json

+    "mini-css-extract-plugin": "^0.9.0",
+    "node-sass": "^6.0.1",
+    "optimize-css-assets-webpack-plugin": "^5.0.3",
+    "pa11y-ci": "^2.4.0",


Some of these we can likely clean out (e.g. we do not test with pa11y-ci). Let's shoot for the minimal possible dependencies

yes. need to do a thorough clean out.

AakashGfude · 2022-01-20T23:53:27Z

@pradyunsg where do you suggest something like a https://github.com/executablebooks/sphinx-book-theme/tree/master/sphinx_book_theme/translations folder should go in terms of the layout system according to sphinx-theme-builder https://sphinx-theme-builder.readthedocs.io/en/latest/reference/filesystem-layout/ .

choldgraf · 2022-01-21T01:21:16Z

I think we could just put it under theme/sphinx_book_theme/translations

AakashGfude · 2022-01-21T06:40:10Z

I think we could just put it under theme/sphinx_book_theme/translations

@choldgraf since we are compiling jsons to *.mo files, and sphinx-theme-builder technically encourages any assets that need to be compiled to be put in src/sphinx_book_theme/assets, so I am a bit confused https://sphinx-theme-builder.readthedocs.io/en/latest/reference/filesystem-layout/. Also, I think we should add the creation of *.mo files in webpack instead of committing them?

pradyunsg · 2022-01-21T08:21:17Z

we are compiling jsons to *.mo files, and sphinx-theme-builder technically encourages any assets that need to be compiled to be put in src/sphinx_book_theme/assets, so I am a bit confused sphinx-theme-builder.readthedocs.io/en/latest/reference/filesystem-layout. Also, I think we should add the creation of *.mo files in webpack instead of committing them?

Hurrah! I wanted to make it easier / explicit for folks to keep compiled assets and uncompiled assets separate -- and I am happy to see that this approach flagged non-CSS/JS stuff like this too! :)

where do you suggest something like a master/sphinx_book_theme/translations folder

I'd suggest leaving them under theme/sphinx_book_theme/ for now, and then coming back around to cleaning that up in a follow up PR.

My heuristic for thinking about such things is: Would it be less work for reviewers if this was in a separate PR? In this case, I think the answer is a yes.

It's technically where they are on main at the moment as well (so... in theory, someone could do {% include "sphinx_book_theme/translations/.gitignore" %} in their templates¹). We can certainly clean this up in well-scoped follow up PR. It makes reviewing easier, and, gives you a clearer list of "what all did I end up doing". :)

Yes, that's why we should move those files out! :) ↩

choldgraf · 2022-01-22T02:02:18Z

I think @pradyunsg has a good intuition here - we should get this PR into a mergeable state ASAP, and we can always use follow-up PRs to clean things up if need be - those won't be nearly as disruptive from a merge conflict standpoint.

AakashGfude · 2022-01-23T08:35:39Z

Yes, totally agree with doing it in follow-up PRs. I guess mostly wanted to bring this issue to light here. Will create a separate issue once this is merged.

I have moved the translations folder to src/sphinx_book_theme instead of leaving them under src/sphinx_book_theme/theme/sphinx_book_theme. Because to make the translations module importable I had to create empty __init__.py in both theme folder and in theme/sphinx_book_theme folder (make them modules as well). To Make the dotted import possible.

As we are importing _convert python file of translations for testing:
from sphinx_book_theme.translations._convert import convert_json

Let me know if I should revert it.

AakashGfude · 2022-01-23T11:29:19Z

Removed theme_dev_mode, config variable, as I think it's not needed anymore, given sphinx-theme-builder. https://github.com/executablebooks/sphinx-book-theme/pull/469/files#diff-827e5b7309121354fc6d5411c429a4a07800bc6abee0db2f47b60c400ec4be99L26

AakashGfude · 2022-01-23T11:30:35Z

Have no idea, how to fix the pre-commit.ci failure 😢.

choldgraf · 2022-01-23T16:26:31Z

Hmm maybe this is related to the "additional dependencies" config? Could we specify the extra dependency in the error message there?

https://github.com/mgedmin/check-manifest#version-control-integration

I wonder if @pradyunsg could weigh in on whether this is somehow related to the sphinx theme builder configuration setup 🤔

pradyunsg · 2022-01-23T16:58:29Z

You don't need check-manifest anymore. That said, that looks like something that's a bit of an Ubuntu mess related to breaking ensurepip?

choldgraf · 2022-01-23T17:26:10Z

Is that because we're not checking in programmatically generated assets into the repo anymore? If that's the case then I am +1 on removing it.

chrisjsewell · 2022-01-23T17:29:02Z

Is that because we're not checking in programmatically generated assets into the repo anymore?

and by extension, what does this mean for MANIFEST.in, is that still being used?

chrisjsewell · 2022-01-23T17:34:10Z

pyproject.toml

+readme = "README.md"
+
+requires-python = ">=3.7"
+dependencies = [


Does this PR now mean docutils 0.17 is supported?

I suspect we'll need to change some CSS still, so I'd assume that it won't be supported in this PR and we shouldn't bump that dependency just yet. I'm working on some HTML/CSS refactoring that I think will add support for docutils .17, but will do that as a follow-up to this PR so that we don't change too many things at the same time.

Yeh exactly, I was pretty sure that was the case; if the CSS still only covers div+class, and not also semantic HTML, then it does not support docutils 0.17 or higher

pradyunsg · 2022-01-23T17:43:56Z

That can be removed as well! sphinx-theme-builder determines what files to include by asking git!

https://github.com/pradyunsg/sphinx-theme-builder/blob/10450cfeb65bc3610a6708ee731069c707978fef/src/sphinx_theme_builder/_internal/distributions.py#L33

chrisjsewell · 2022-01-23T17:44:56Z

I guess that's similar to flit 👍

pradyunsg · 2022-01-23T17:45:06Z

Basically, the sdist ends up being the same as a git archive or whatever is the equivalent of downloading from the GitHub repository after generating the assets.

pradyunsg · 2022-01-23T17:49:57Z

Yup, basically the same as flit. I'm pretty sure I copy-pasted some code for that too.

choldgraf · 2022-01-23T19:24:57Z

(side comment - I think all of these more recent developments in Python packaging are pretty cool, thanks @pradyunsg for all of the work you're doing in that space :-) )

AakashGfude · 2022-01-23T23:35:49Z

Basically, the sdist ends up being the same as a git archive or whatever is the equivalent of downloading from the GitHub repository after generating the assets.

This is pretty cool, I will remove the MANIFEST file.

AakashGfude · 2022-01-24T12:14:08Z

Looks okayish to review.
I still need to update the docs though.

choldgraf

This looks really nice to me! I checked out this PR locally and was able to successfully run tox -e docs-live 🎉

I've left a few (relatively minor) comments throughout, let me know what you think.

I agree that the main thing left to do is document this infrastructure so that others know what's going on. I don't think we want to block this PR on having a ton of docs. How about:

Provide some basic information about how to build the theme locally with tox
Provide some basic information about the sphinx-theme-builder
Provide some basic information about the webpack and JS-specific files that you've added so others know what's going on

You can use the pydata contributor documentation for inspiration about what stuff to include.

Once we have those basic pieces there, so that the contributing documentation is "technically correct" even if not totally complete, then I think we should merge this in and start iterating in more focused PRs from there.

src/sphinx_book_theme/__init__.py

choldgraf · 2022-01-25T00:51:19Z

src/sphinx_book_theme/assets/styles/_footer.scss

@@ -2,9 +2,9 @@
 * Footer *
 *********************************************/
 footer {
-    padding-left: 0;


Just to confirm - have you changed anything in the CSS files other than either moving them or updating their formatting in a way that doesn't change end behavior? If you have changed something, can you briefly describe what you've changed?

I have added a few styles, for the margin directive in this commit 4fb5154 , which I pointed out in this comment #469 (comment). Still need to find the issue which was created for this. I will update that issue once I find it.

tox.ini

webpack.config.js

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

…/sphinx-book-theme into sphinx-theme-builder

AakashGfude · 2022-01-25T02:35:40Z

Provide some basic information about how to build the theme locally with tox

Provide some basic information about the sphinx-theme-builder

Provide some basic information about the webpack and JS-specific files that you've added so others know what's going on

@choldgraf I updated the contributing guide docs/contributing.md, which briefly touches or just mentions all these points. I reckon we should add more, but we can do it in a follow-up PR?

Will check the translations README, and update it as well.

choldgraf · 2022-01-25T04:32:31Z

Those contributing docs look good to me 👍 - anything else you want to tackle in this PR? Otherwise I'd say we leave it open until tomorrow in case others want to make comments, and merge it in unless there are any objections. This PR touches a lot of stuff so it'd be good to get it in so that we can start iterating.

AakashGfude · 2022-01-25T04:37:37Z

Sounds good @choldgraf. I think I am done with any edits/additions in this PR.

src/sphinx_book_theme/assets/scripts/index.js

choldgraf · 2022-01-25T22:51:20Z

OK, I took another pass and added in one quick comment, there are likely still some things that we need to clean up but I think we can do that in follow-ups. The styles look good to me in general, but there seem to be a few tweaks to make (maybe related to docutils). I will tackle those in the refactor PR (#471) and we can hold off on closing #392 until that PR is merged as well.

I'll merge this PR so that we can start iterating a bit from here.

pradyunsg

Hah. I had clicked the wrong button.

pradyunsg · 2022-01-25T08:33:49Z

docs/contributing.md

-   $ pip install tox
-   ```
-3. **Build the documentation**. You can use the following `tox` command:
+1. **Make your changes in `src/sphinx_book_theme/assets/`**. This folder contains all of the SCSS and Javascript that are used in this site. You should edit the files here, and they will be built and included with the site in the `sphinx_book_theme/` folder at build time.


This seems... odd. Contributors might be modifying the templates or Python code as well.

good point - I can take a pass at the contributing docs in general as well (or spot check other changes as needed) in #472

Thanks, @pradyunsg for pointing this out.

choldgraf reviewed Jan 18, 2022

View reviewed changes

AakashGfude changed the title ~~Switch to sphinx-theme-builder~~ [ENH] Switch to sphinx-theme-builder Jan 21, 2022

chrisjsewell reviewed Jan 23, 2022

View reviewed changes

AakashGfude added 9 commits January 24, 2022 11:04

using sphinx-theme-builder

7d5616e

correcting init and removing theme_dev_mode

bb19e68

removing manifest check

892a3f4

using tox instead of nox

b1aa911

commenting test for translations for now

939db94

tests workflow

599a680

updating git workflow

91a82c1

tox.ini update

80423c7

including *.mo files in MANIFEST for now

d85f0e4

AakashGfude added 2 commits January 24, 2022 22:48

javascript corrections, code/compilation

a30c858

javascript corrections

8627fbe

AakashGfude marked this pull request as ready for review January 24, 2022 12:11

choldgraf mentioned this pull request Jan 25, 2022

[WIP] REFACTOR: Re-work HTML structure and CSS #471

Closed

choldgraf reviewed Jan 25, 2022

View reviewed changes

AakashGfude and others added 4 commits January 25, 2022 13:21

updated contributing guide, and removed

fb5ed0a

Update tox.ini

83e0226

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

deleting commented code in __init__.py

80d0955

Merge branch 'sphinx-theme-builder' of https://github.com/AakashGfude…

10bbb79

…/sphinx-book-theme into sphinx-theme-builder

AakashGfude added 4 commits January 25, 2022 14:47

adding comments on scss loaders in webpack

d7a1a67

translations readme update

664a77e

adding comments on scss loader pipeline in webpack

2d6b8e2

corrections/additions to contributing.md

48c43c1

adding choldgraf's email address in pyproject.toml

9f0a374

choldgraf reviewed Jan 25, 2022

View reviewed changes

src/sphinx_book_theme/assets/scripts/index.js Show resolved Hide resolved

Update src/sphinx_book_theme/assets/scripts/index.js

9fc1186

choldgraf approved these changes Jan 25, 2022

View reviewed changes

choldgraf changed the title ~~[ENH] Switch to sphinx-theme-builder~~ MAINT: Switch to sphinx-theme-builder Jan 25, 2022

choldgraf merged commit ca4f765 into executablebooks:master Jan 25, 2022

choldgraf mentioned this pull request Jan 26, 2022

‼️ REFACTOR: HTML and CSS restructuring #472

Merged

15 tasks

pradyunsg reviewed Jan 26, 2022

View reviewed changes

AakashGfude mentioned this pull request Jan 26, 2022

Switch our build system to use the sphinx-theme-builder #417

Closed

AakashGfude self-assigned this Jan 28, 2022

AakashGfude deleted the sphinx-theme-builder branch January 28, 2022 02:14

AakashGfude mentioned this pull request Feb 2, 2022

Put uncompiled assets in translations in a proper directory adhering to sphinx-theme-builder #475

Closed

MAINT: Switch to sphinx-theme-builder #469

MAINT: Switch to sphinx-theme-builder #469

Conversation

AakashGfude commented Jan 18, 2022 • edited

choldgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AakashGfude commented Jan 20, 2022

choldgraf commented Jan 21, 2022 • edited

AakashGfude commented Jan 21, 2022 • edited

pradyunsg commented Jan 21, 2022 • edited

Footnotes

choldgraf commented Jan 22, 2022

AakashGfude commented Jan 23, 2022 • edited

AakashGfude commented Jan 23, 2022 • edited

AakashGfude commented Jan 23, 2022 • edited

choldgraf commented Jan 23, 2022 • edited

pradyunsg commented Jan 23, 2022 • edited

choldgraf commented Jan 23, 2022

chrisjsewell commented Jan 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell Jan 23, 2022 • edited

Choose a reason for hiding this comment

pradyunsg commented Jan 23, 2022

chrisjsewell commented Jan 23, 2022

pradyunsg commented Jan 23, 2022

pradyunsg commented Jan 23, 2022

choldgraf commented Jan 23, 2022

AakashGfude commented Jan 23, 2022

AakashGfude commented Jan 24, 2022

choldgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AakashGfude commented Jan 25, 2022 • edited

choldgraf commented Jan 25, 2022

AakashGfude commented Jan 25, 2022

choldgraf commented Jan 25, 2022 • edited

pradyunsg 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

AakashGfude commented Jan 18, 2022 •

edited

choldgraf commented Jan 21, 2022 •

edited

AakashGfude commented Jan 21, 2022 •

edited

pradyunsg commented Jan 21, 2022 •

edited

AakashGfude commented Jan 23, 2022 •

edited

AakashGfude commented Jan 23, 2022 •

edited

AakashGfude commented Jan 23, 2022 •

edited

choldgraf commented Jan 23, 2022 •

edited

pradyunsg commented Jan 23, 2022 •

edited

chrisjsewell commented Jan 23, 2022 •

edited

chrisjsewell Jan 23, 2022 •

edited

AakashGfude commented Jan 25, 2022 •

edited

choldgraf commented Jan 25, 2022 •

edited