Skip to content

Convert docs theme to minima and add template elements for baseline site #367

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GeauxJD
Copy link

@GeauxJD GeauxJD commented Aug 5, 2025

Resolves #341

Updates to Jekyll's default minima theme
Adds header and footer content to templates
Updates to OpenSSF brand guidelines fonts
adds front-matter to 3 of the markdown pages for navigation functionality
Corrects a minor grammar issue in index.md

Copy link
Contributor

@funnelfiasco funnelfiasco left a comment

Choose a reason for hiding this comment

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

This looks pretty good overall. I think we'll want to include the Gemfile* ...files, though. We should also update the spellcheck config so it doesn't complain about the non-words that it's complaining about (I wish it were better about distinguishing things here, but alas)

Once we merge this, I'll need to update the release instructions to include modifying the current version to have the nav-title.

.gitignore Outdated
Comment on lines 6 to 7
Gemfile
Gemfile.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to ignore these? Seems like it'd be beneficial for publishing (and for folks doing local builds, too)

Copy link
Contributor

Choose a reason for hiding this comment

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

https://baseline.openssf.org/versions/2025-02-25#osps-qa-0201 suggests that we should probably have at least the Gemfile (and I'd recommend both)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's an interesting take... I don't read it that way. Since QA-02 is about "the software's direct dependencies," I wouldn't expect that dependencies for the documentation website would be covered by the test requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the docs website isn't strictly covered by QA-02, but the spirit of QA-02 applies. And, if nothing else, it makes life a lot more convenient for local builds, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the output (released artifact) is the website, then the Jekyll configuration (and the go templating) are both the software used to produce the output. If that's not the intent, then we should figure out how to clarify.

On the other hand, the version of Jekyll used to generate the website is actually specified by https://github.com/ossf/security-baseline/blob/main/.github/workflows/web-publish.yml#L36, so maybe this is just "development cruft" that could be ignored as it only impacts contributors attempting to build a local copy.

@evankanderson
Copy link
Contributor

evankanderson commented Aug 5, 2025

This looks pretty good overall. I think we'll want to include the Gemfile* ...files, though. We should also update the spellcheck config so it doesn't complain about the non-words that it's complaining about (I wish it were better about distinguishing things here, but alas)

We could spell-check only .md and .yaml files: https://github.com/streetsidesoftware/cspell-action?tab=readme-ov-file#usage

Co-authored-by: Ben Cotton <[email protected]>
Signed-off-by: Eddie Knight <[email protected]>
@@ -1,3 +1,7 @@
---
nav-title: Current Version
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nav-title: Current Version
nav-title: 2025-02-25 (Current)

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if the 2025-02-25 as the title in a navbar link is going to be very intuitive for someone who is new to the project

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jeff. We could do "Current Version (2025-02-05)" instead, but I think for navbar purposes "Current version" is sufficient.

.gitignore Outdated
Comment on lines 6 to 7
Gemfile
Gemfile.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

https://baseline.openssf.org/versions/2025-02-25#osps-qa-0201 suggests that we should probably have at least the Gemfile (and I'd recommend both)

@GeauxJD
Copy link
Author

GeauxJD commented Aug 6, 2025

This looks pretty good overall. I think we'll want to include the Gemfile* ...files, though. We should also update the spellcheck config so it doesn't complain about the non-words that it's complaining about (I wish it were better about distinguishing things here, but alas)

We could spell-check only .md and .yaml files: https://github.com/streetsidesoftware/cspell-action?tab=readme-ov-file#usage

Can I get some help from someone on the spellcheck piece? I'm not familiar with cspell or how to configure it.

Signed-off-by: Ben Cotton <[email protected]>
@funnelfiasco
Copy link
Contributor

Can I get some help from someone on the spellcheck piece? I'm not familiar with cspell or how to configure it.

I just added commit 4661296 which fixes it.

I went with a mixed approach here, and opted to forego the action-based config change so that it will work with local spell checking. Specifically, I excluded the docs/assets/ directory from checking, since the contents of that directory are unlikely to need actual spell checking. I also added hyperpage and webfonts to the known words in .project-words.txt because I didn't want to exclude the html files, which might have legitimate spell check failures.

Is it the best approach? Perhaps, perhaps not.

@GeauxJD
Copy link
Author

GeauxJD commented Aug 6, 2025

OK PR uodated to add in the Gemfile and Gemfile.lock

@GeauxJD GeauxJD requested a review from funnelfiasco August 6, 2025 15:58
funnelfiasco
funnelfiasco previously approved these changes Aug 6, 2025
Copy link
Contributor

@funnelfiasco funnelfiasco left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @GeauxJD!

trumant
trumant previously approved these changes Aug 6, 2025
@trumant trumant dismissed stale reviews from funnelfiasco and themself via 5d0ef52 August 6, 2025 17:48
Copy link
Contributor

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

A few minor nits, but I'm willing to merge as-is.

<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1">

{% seo %}
<link rel="stylesheet" href="{{ "/assets/css/style.css?v=" | append: site.github.build_revision | relative_url }}">
{% include head-custom.html %}
<link rel="stylesheet" href="{{ '/assets/css/style.css' | relative_url }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where custom-head.html is being included now?

Also, it looks like the previous stylesheet link had some cache-busting parameters for when the CSS was updated. I don't care strongly, but further updates to the CSS may take longer to show up (and I'm fine with that).

Copy link
Author

Choose a reason for hiding this comment

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

i think you are right that custom-head.html is a legacy file from copying some things in from a setup of these templates elsewhere. we can probably do a follow-on issue to test removing that file and confirm nothing breaks.

Comment on lines +10 to +12
<!--[if lt IE 9]>
<script src="https://cdnjs.cloudflare.com/ajax/libs/html5shiv/3.7.3/html5shiv.min.js"></script>
<![endif]-->
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're running IE < 9.0, you're running a browser from 2009 or earlier, and it's even out of support on Windows Embedded POSReady 2009 with extended support updates. I think you can drop this.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah this came straight from the minima theme defaults, i'd be ok with us testing removal in a follow-on issue

Comment on lines +12 to +14
* https://fonts.google.com/specimen/Teko
- This *was* the recommended heading font, but is no longer.
Many found it hard to read.
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have this font -- remove this?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, can remove this. @funnelfiasco let me know if you want me to do some more commits now to address some of these nits or if we want to followup with another PR later for cleanups.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well address them now, unless you want to juice your stats :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update GitHub pages template
5 participants