-
Notifications
You must be signed in to change notification settings - Fork 5
Use Primer Spec #61
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
Use Primer Spec #61
Conversation
seshrs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! But you’ll also need to add a GitHub Pages action in the .github folder, and change the repo settings to deploy via Action. More details here: https://github.com/seshrs/build-primer-spec-action#setting-up-github-pages-deployment
|
Thanks for the pointers! I'm curious what the custom action does differently from GitHub's default "Deploy from a branch" action? I tried the default on my fork and it seems to render the spec without issue. |
|
(Oh sorry I thought I replied.) The TLDR is that the custom action guarantees that your website can use all of Primer Spec features. Currently, the only value-add is that the action generates downloadable PDFs for each page; but in future, we could theoretically add more features that rely on build-time customizations. |
|
Should we also include a |
Adding those is tricky. You gotta use certain versions of Ruby and bundler to be compatible with CI. I just pushed them. |
The action is failing on my fork. Did I misconfigure it or do the versions need to be changed? |
Now that custom GitHub Actions and Pages deployment is a thing, I wonder if I should rewrite Primer Spec to remove our dependency on Jekyll… Btw the Gemfile is only needed to preview the pages locally: https://eecs485staff.github.io/primer-spec/docs/USAGE_ADVANCED.html#previewing-locally |
Hmm… I can try looking later today. (Another reason I want to move away from Jekyll!) |
+1 that's along the lines I was thinking, in terms of consistency with our other repos and the contributing instructions for local preview. (Even though it may not really be that crucial for local previews on the UTF tutorial.) |
|
Maybe just ripping out the lockfile is the way to go! |
|
Is it a bad idea to use the same |
I believe this will work fine. The issue appears to be that a freshly generated But, using the |
|
That should be mostly fine. It would also be fine to skip the lockfile. |
|
The action works now, so I think this is ready to merge. Whoever merges just needs to change the repository settings to "deploy via action" since I don't have access to that. |
jamesjuett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll merge and update the settings.
Closes #45. Changed one of the notes into a "callout" box. Not sure if that was a good choice or if there were other places where it would useful to do as well.