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

docs: update to latest hugo-theme-techdoc #1843

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

krancour
Copy link
Contributor

@krancour krancour commented Feb 23, 2022

Fixes #1385

This rips out our Frankenstein customizations to the Hugo Techdoc theme and replaces it with the latest version using a submodule. This is apparently how themes are meant to be used.

I've updated our customizations to work with recent changes to the theme. #1385 is fixed in the process. Those customizations are all applied outside the themes/ folder as is recommended.

@krancour krancour self-assigned this Feb 23, 2022
@krancour krancour marked this pull request as draft February 23, 2022 20:23
@krancour krancour added this to the v2.4.0 milestone Feb 23, 2022
@krancour krancour added the documentation Improvements or additions to documentation label Feb 23, 2022
@netlify
Copy link

netlify bot commented Feb 23, 2022

✔️ Deploy Preview for brigade-docs ready!

🔨 Explore the source changes: 2efb18b

🔍 Inspect the deploy log: https://app.netlify.com/sites/brigade-docs/deploys/62182d21c49429000868490f

😎 Browse the preview: https://deploy-preview-1843--brigade-docs.netlify.app

{{- partial "custom-css.html" . -}}
{{- template "_internal/opengraph.html" . -}}
{{- template "_internal/twitter_cards.html" . -}}
{{- template "_internal/schema.html" . -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is copied from the theme and minimally modified to include include our favicon and custom stylesheet. Hugo favors this file over the original in themes/...

@krancour krancour force-pushed the upgrade-techdoc branch 2 times, most recently from 2c0f6f8 to f6e0407 Compare February 24, 2022 20:59
@@ -1,30 +1,30 @@
{{$currentNode := .}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is copied from the theme and minimally modified to favor using each page's titleLink over its title when generating the menu. Hugo favors this file over the original in themes/...

@@ -94,3 +68,6 @@
});
};
</script>
<!-- END: Google Analytics -->
{{- end -}}
{{- end -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is copied from the theme and minimally modified to require users consent to the use of cookies before Google Analytics is activated. Hugo favors this file over the original in themes/...

This is consistent with the behavior prior to this PR. The only difference is that the hack is now part of our site code rather than a modification to the theme.

@@ -0,0 +1 @@
<!-- This is deliberately empty to override something we don't want from the theme. -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This overrides the file of the same name found in themes/...

The original uses a <noscript> element to activate Google Analytics in an invisible <iframe> when JS is disabled in the user's browser.

We don't want that because we don't want to activate Google Analytics unless the user explicitly consents to the use of tracking cookies, and we cannot gather that consent without JS enabled, so we simply have to live without GA in this case.

@krancour krancour marked this pull request as ready for review February 24, 2022 21:13
@krancour krancour requested review from vdice and flynnduism February 24, 2022 21:13
@krancour krancour force-pushed the upgrade-techdoc branch 4 times, most recently from d1b0b84 to 35e3d23 Compare February 25, 2022 01:05
Signed-off-by: Kent Rancourt <kent.rancourt@microsoft.com>
Copy link
Contributor

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Rather than going through all of the changed files in detail, I checked out the deploy preview and all looks great to me.

It sounds like this changeset gets us up-to-date and the internal doc workings vastly cleaned up, which all sounds excellent.

Noticed some notes around Google Analytics -- I'll defer to @krancour to make sure all is working as expected.

Otherwise, LGTM from me!

@krancour
Copy link
Contributor Author

Noticed some notes around Google Analytics -- I'll defer to @krancour to make sure all is working as expected.

@vdice it is for sure working identically to before. Using the browser's developer tools, I was able to compare before/after and verify that Google Analytics is now incorporated into the site only after a user explicitly consents to cookies -- same as before. The only thing that's up in the air is that I'm missing access to view the collected data, but we'll work that out.

@krancour krancour merged commit d62e3b0 into brigadecore:main Feb 25, 2022
@krancour krancour deleted the upgrade-techdoc branch February 25, 2022 18:45
krancour added a commit to krancour/brigade that referenced this pull request Mar 1, 2022
Signed-off-by: Kent Rancourt <kent.rancourt@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On smaller viewports, sidebar overlaps with main content
2 participants