Update docsy (hugo theme) git submodule #1295

anoadragon453 · 2022-10-19T11:35:32Z

This PR updates our git submodule checkout of docsy to matrix-org/docsy@a0032f8, which is the latest current commit of google/docsy + some changes to locally-load fonts/css/js.

The previous version of docsy was >1 year old.

A few visual changes that I've noticed. These look fine and don't require fixing IMO:

(Table cell vertical text alignment) Before

After

(Breadcrumbs) Before

After

(Rate-limit/Auth text) Before

After

This change does not require a newsfragment.

Preview: https://pr1295--matrix-spec-previews.netlify.app

anoadragon453 · 2022-10-27T15:44:08Z

I'm going to rework this PR to fix matrix-org/docsy#1.

turt2live

clearing review while draft

Update to matrix-org/docsy@a0032f8, which has a few changes on top of google/docsy.

Otherwise table-layout has no effect. I'm not sure why this broke, possibly google/docsy#1255.

This fixes matrix-org/docsy#1.

anoadragon453 · 2022-10-27T16:20:54Z

This has been updated to the latest https://github.com/matrix-org/docsy/ (as of today). 343edc7 prevents all google fonts from loading while still loading Inter. The change also allowed removing one downstream change in our fork of docsy.

.github/workflows/main.yml

richvdh · 2022-11-01T14:42:08Z

.github/workflows/main.yml

+        run: |
+          cd themes/docsy
+          npm i
+          npm i --save-dev autoprefixer postcss-cli postcss


npm i --save-dev autoprefixer postcss-cli postcss

this doesn't look right at all. This is modifying the package.json of docsy to say that these libraries are dev-time dependencies of docsy. If that's true, then it should be checked into git rather than done on the fly... but I don't think it's true.

The suggestion at https://github.com/google/docsy/#prerequisites is to do it in the Hugo site which uses docsy (ie, matrix-spec), not docsy itself. And it's already been done in matrix-spec: https://github.com/matrix-org/matrix-spec/blob/main/package.json#L21-L26

Right, it seems I got myself in quite a confused state with the changes here. For clarity, note that we install docsy as a git submodule.

In the past, docsy used to have vendored submodules in its repo for bootstrap and Font Awesome. That is no longer the case with the current state of the theme. Now, bootstrap and font awesome are pulled in via npm.

So, in the past we got away with simply running git submodule update --init --recursive in the root of matrix-spec repo to get access to these dependencies. Now, we need to run npm i in the themes/docsy directory to get them.

I had gotten confused as scss files were failing to be built when running hugo serve. Then, after running npm i --save-dev autoprefixer postcss-cli postcss in themes/hugo, hugo serve worked! But what was actually the thing that made it work was installing the deps listed in themes/hugo/package.json.

So yes: this CI is wrong, and in addition our instructions to the build the repo need to be updated to include the extra npm i.

Done in 3313b6a.

richvdh · 2022-11-01T14:48:45Z

assets/scss/_variables_project.scss

@@ -37,8 +37,17 @@ $table-row-alternate: $secondary-lightest-background;
 $table-row-default: $secondary-lighter-background;

 /*
- Opt to serve fonts locally by overriding web-font-path to be a non-google fonts URL.
- This is only possible with our modified docsy theme: https://github.com/matrix-org/docsy
+ Disable loading any fonts from Google Fonts.


Suggested change

Disable loading any fonts from Google Fonts.

* Configure docsy to use the default bootstrap fonts instead of Google Fonts.

* See https://www.docsy.dev/docs/adding-content/lookandfeel/#fonts

Note that no fonts are imported by bootstrap - bootstrap tries to load fonts from the system: https://getbootstrap.com/docs/5.2/content/reboot/#native-font-stack

$td-enable-google-fonts will disable fetching fonts from Google Fonts, and use system fonts instead. The list of system fonts is configured by $font-family-sans-serif below. The comment about "The list of fonts ..." tried to explain this, but I'll clarify it to mention $font-family-sans-serif explicitly. While the contents of $font-family-sans-serif looks like what bootstrap defines, it's originally from docsy's source code.

We have to override it (with Inter at the front) in order to load Inter.

Done in a101159.

Ok, the thing I really wanted to see here is a link to the place where td-enable-google-fonts is defined, which we still don't have. The interesting point here is that it's exposed by docsy rather than bootstrap.

I think my original suggestion is ok, with s/bootstrap/system/ ?

Ah, yes we definitely should add a link to that piece of documentation.

I think my original suggestion is ok, with s/bootstrap/system/ ?

Yes, this looks good on a re-read.

Added the modified suggestion in 4473805.

assets/scss/_variables_project.scss

assets/scss/custom.scss

richvdh · 2022-11-01T14:55:18Z

Also: changelog please

This reverts commit 5c52d4f.

This commit adds a bit of helper code to the top-level package.json to always ensure that the `themes/docsy` submodule is checked out and that its nodejs dependencies are installed. Thus, we can remove the need to download git submodules directly. Instead they will be loaded when running `npm i`. We also don't need to load submodules recursively anymore, as docsy has removed any submodules from its repo since the last time we updated the theme. The `--depth 1` bit of `git submodule update --init --depth 1` tells git to only check out the latest commit of any submodules - saving bandwidth and space.

anoadragon453

Also: changelog please

CONTRIBUTING.md mentions that:

Changes that do not change the spec, such as changes to the build script, formatting, CSS, etc should not get a news fragment.

So I'm not sure if this PR needs one?

anoadragon453 · 2022-11-01T17:30:35Z

.github/workflows/main.yml

+        run: |
+          cd themes/docsy
+          npm i
+          npm i --save-dev autoprefixer postcss-cli postcss


Right, it seems I got myself in quite a confused state with the changes here. For clarity, note that we install docsy as a git submodule.

In the past, docsy used to have vendored submodules in its repo for bootstrap and Font Awesome. That is no longer the case with the current state of the theme. Now, bootstrap and font awesome are pulled in via npm.

So, in the past we got away with simply running git submodule update --init --recursive in the root of matrix-spec repo to get access to these dependencies. Now, we need to run npm i in the themes/docsy directory to get them.

I had gotten confused as scss files were failing to be built when running hugo serve. Then, after running npm i --save-dev autoprefixer postcss-cli postcss in themes/hugo, hugo serve worked! But what was actually the thing that made it work was installing the deps listed in themes/hugo/package.json.

So yes: this CI is wrong, and in addition our instructions to the build the repo need to be updated to include the extra npm i.

anoadragon453 · 2022-11-01T18:09:48Z

assets/scss/_variables_project.scss

@@ -37,8 +37,17 @@ $table-row-alternate: $secondary-lightest-background;
 $table-row-default: $secondary-lighter-background;

 /*
- Opt to serve fonts locally by overriding web-font-path to be a non-google fonts URL.
- This is only possible with our modified docsy theme: https://github.com/matrix-org/docsy
+ Disable loading any fonts from Google Fonts.


Note that no fonts are imported by bootstrap - bootstrap tries to load fonts from the system: https://getbootstrap.com/docs/5.2/content/reboot/#native-font-stack

$td-enable-google-fonts will disable fetching fonts from Google Fonts, and use system fonts instead. The list of system fonts is configured by $font-family-sans-serif below. The comment about "The list of fonts ..." tried to explain this, but I'll clarify it to mention $font-family-sans-serif explicitly. While the contents of $font-family-sans-serif looks like what bootstrap defines, it's originally from docsy's source code.

We have to override it (with Inter at the front) in order to load Inter.

assets/scss/custom.scss

assets/scss/_variables_project.scss

anoadragon453 · 2022-11-01T18:35:36Z

.github/workflows/main.yml

+        run: |
+          cd themes/docsy
+          npm i
+          npm i --save-dev autoprefixer postcss-cli postcss


Done in 3313b6a.

anoadragon453 · 2022-11-01T18:35:42Z

assets/scss/_variables_project.scss

@@ -37,8 +37,17 @@ $table-row-alternate: $secondary-lightest-background;
 $table-row-default: $secondary-lighter-background;

 /*
- Opt to serve fonts locally by overriding web-font-path to be a non-google fonts URL.
- This is only possible with our modified docsy theme: https://github.com/matrix-org/docsy
+ Disable loading any fonts from Google Fonts.


Done in a101159.

anoadragon453 · 2022-11-01T18:39:48Z

package.json

+    "get:submodule": "git submodule update --init --depth 1",
+    "_prepare:docsy": "cd themes/docsy && npm install",
+    "prepare": "npm run get:submodule && npm run _prepare:docsy",


This is taken from docsy's docs: https://www.docsy.dev/docs/get-started/other-options/#for-an-existing-site

Unfortunately, we can't add comments to package.json files...

richvdh · 2022-11-03T07:22:19Z

Also: changelog please

CONTRIBUTING.md mentions that:

Changes that do not change the spec, such as changes to the build script, formatting, CSS, etc should not get a news fragment.

So I'm not sure if this PR needs one?

CONTRIBUTING.md lies. We have an internal changelog section now for this sort of thing.

richvdh

thanks for doing all that digging. still a couple of nits.

README.md

richvdh · 2022-11-03T07:26:08Z

assets/scss/_variables_project.scss

@@ -37,8 +37,17 @@ $table-row-alternate: $secondary-lightest-background;
 $table-row-default: $secondary-lighter-background;

 /*
- Opt to serve fonts locally by overriding web-font-path to be a non-google fonts URL.
- This is only possible with our modified docsy theme: https://github.com/matrix-org/docsy
+ Disable loading any fonts from Google Fonts.


Ok, the thing I really wanted to see here is a link to the place where td-enable-google-fonts is defined, which we still don't have. The interesting point here is that it's exposed by docsy rather than bootstrap.

I think my original suggestion is ok, with s/bootstrap/system/ ?

assets/scss/custom.scss

SCSS's documentation seems to suggest a similar style: https://sass-lang.com/documentation/syntax/comments

anoadragon453

I've added a changelog entry as well (as .feature, which I thought fit best amongst the options, but feedback welcome!).

README.md

anoadragon453 · 2022-11-08T16:13:15Z

assets/scss/_variables_project.scss

@@ -37,8 +37,17 @@ $table-row-alternate: $secondary-lightest-background;
 $table-row-default: $secondary-lighter-background;

 /*
- Opt to serve fonts locally by overriding web-font-path to be a non-google fonts URL.
- This is only possible with our modified docsy theme: https://github.com/matrix-org/docsy
+ Disable loading any fonts from Google Fonts.


Ah, yes we definitely should add a link to that piece of documentation.

I think my original suggestion is ok, with s/bootstrap/system/ ?

Yes, this looks good on a re-read.

README.md

anoadragon453 · 2022-11-08T16:45:48Z

assets/scss/_variables_project.scss

@@ -37,8 +37,17 @@ $table-row-alternate: $secondary-lightest-background;
 $table-row-default: $secondary-lighter-background;

 /*
- Opt to serve fonts locally by overriding web-font-path to be a non-google fonts URL.
- This is only possible with our modified docsy theme: https://github.com/matrix-org/docsy
+ Disable loading any fonts from Google Fonts.


Added the modified suggestion in 4473805.

assets/scss/custom.scss

richvdh

anoadragon453 · 2022-11-08T17:28:36Z

anoadragon453 force-pushed the anoa/update_docsy branch from b8c5602 to 9417630 Compare October 25, 2022 16:32

anoadragon453 marked this pull request as ready for review October 25, 2022 16:34

anoadragon453 requested a review from a team as a code owner October 25, 2022 16:34

anoadragon453 marked this pull request as draft October 27, 2022 15:44

turt2live reviewed Oct 27, 2022

View reviewed changes

anoadragon453 added 3 commits October 27, 2022 17:08

Update docsy to google/docsy@90843e3 + changes

fba8967

Update to matrix-org/docsy@a0032f8, which has a few changes on top of google/docsy.

Update GHA to install docsy CSS build dependencies

5c52d4f

force table to display as a table

0907cf7

Otherwise table-layout has no effect. I'm not sure why this broke, possibly google/docsy#1255.

anoadragon453 force-pushed the anoa/update_docsy branch from 9417630 to 86a5d99 Compare October 27, 2022 16:15

Disable all google fonts for docsy, load inter directly

343edc7

This fixes matrix-org/docsy#1.

anoadragon453 force-pushed the anoa/update_docsy branch from 86a5d99 to 343edc7 Compare October 27, 2022 16:18

anoadragon453 marked this pull request as ready for review October 27, 2022 16:18

anoadragon453 requested a review from a team October 27, 2022 16:21

anoadragon453 mentioned this pull request Oct 27, 2022

docsy theme still causes outgoing connections to Google Fonts for Arabic and Hebrew languages matrix-org/docsy#1

Closed

richvdh requested changes Nov 1, 2022

View reviewed changes

anoadragon453 added 3 commits November 1, 2022 17:00

Revert "Update GHA to install docsy CSS build dependencies"

6e4c184

This reverts commit 5c52d4f.

Clarify comment about loading Inter

a101159

anoadragon453 commented Nov 1, 2022

View reviewed changes

anoadragon453 requested a review from richvdh November 1, 2022 18:38

anoadragon453 commented Nov 1, 2022

View reviewed changes

richvdh requested changes Nov 3, 2022

View reviewed changes

anoadragon453 added 4 commits November 8, 2022 16:21

Mention that 'npm i' will install git submodules

007bc8e

Update SCSS comment formatting to match the repo

a2c9580

SCSS's documentation seems to suggest a similar style: https://sass-lang.com/documentation/syntax/comments

Link to docsy docs on $td-enable-google-fonts

4473805

Add long-winded comment about why display: table is needed

f261919

Add internal changelog

90b77e1

anoadragon453 commented Nov 8, 2022

View reviewed changes

anoadragon453 requested a review from richvdh November 8, 2022 16:47

richvdh approved these changes Nov 8, 2022

View reviewed changes

anoadragon453 merged commit fd41d9d into main Nov 8, 2022

anoadragon453 deleted the anoa/update_docsy branch November 8, 2022 17:29

clokep pushed a commit to clokep/matrix-spec that referenced this pull request May 3, 2023

Update docsy (hugo theme) git submodule (matrix-org#1295)

85d6ea7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update docsy (hugo theme) git submodule #1295

Update docsy (hugo theme) git submodule #1295

anoadragon453 commented Oct 19, 2022 •

edited by github-actions bot

Loading

anoadragon453 commented Oct 27, 2022

turt2live left a comment

anoadragon453 commented Oct 27, 2022 •

edited

Loading

richvdh Nov 1, 2022

anoadragon453 Nov 1, 2022

anoadragon453 Nov 1, 2022

richvdh Nov 1, 2022

anoadragon453 Nov 1, 2022

anoadragon453 Nov 1, 2022

richvdh Nov 3, 2022

anoadragon453 Nov 8, 2022

anoadragon453 Nov 8, 2022

richvdh commented Nov 1, 2022

anoadragon453 left a comment

anoadragon453 Nov 1, 2022

anoadragon453 Nov 1, 2022

anoadragon453 Nov 1, 2022

anoadragon453 Nov 1, 2022

anoadragon453 Nov 1, 2022

richvdh commented Nov 3, 2022

richvdh left a comment

richvdh Nov 3, 2022

anoadragon453 left a comment

anoadragon453 Nov 8, 2022

anoadragon453 Nov 8, 2022

richvdh left a comment

anoadragon453 commented Nov 8, 2022

	Disable loading any fonts from Google Fonts.
	* Configure docsy to use the default bootstrap fonts instead of Google Fonts.
	* See https://www.docsy.dev/docs/adding-content/lookandfeel/#fonts

Update docsy (hugo theme) git submodule #1295

Update docsy (hugo theme) git submodule #1295

Conversation

anoadragon453 commented Oct 19, 2022 • edited by github-actions bot Loading

anoadragon453 commented Oct 27, 2022

turt2live left a comment

Choose a reason for hiding this comment

anoadragon453 commented Oct 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Nov 1, 2022

anoadragon453 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

richvdh commented Nov 3, 2022

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoadragon453 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

anoadragon453 commented Nov 8, 2022

anoadragon453 commented Oct 19, 2022 •

edited by github-actions bot

Loading

anoadragon453 commented Oct 27, 2022 •

edited

Loading