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

Set correct source directory for jekyll gh-action page and add {{ site.ur l}} to fix links and styling #123

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

danadoherty639
Copy link

@danadoherty639 danadoherty639 commented Jul 2, 2024

Summary

The jekyll gh-pages action "sources" its documentation from the root "/" of the repository however respec-puppet holds its documentation under a sub-directory /docs. This points the action to the correct directory to build the documention from with more advanced configuration options under /docs/config.yml. This PR also fixes the issue with the broken links by using Jekyll's global variable site.url configured in /docs/config.yaml. The default css theming is expected to be served from the default root directory and ours is respec-puppet and the site.url is used to fixed this and serve the css from the correct location.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.80%. Comparing base (2c8b9cf) to head (5bc8f9e).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #123   +/-   ##
=======================================
  Coverage   86.80%   86.80%           
=======================================
  Files          35       35           
  Lines        2092     2092           
=======================================
  Hits         1816     1816           
  Misses        276      276           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/index.html Outdated Show resolved Hide resolved
docs/index.html Outdated Show resolved Hide resolved
docs/_config.yml Outdated Show resolved Hide resolved
docs/index.html Outdated
@@ -54,7 +56,7 @@ <h5>(The rest is a bit more difficult though)</h5>
$ cd your-module
$ rspec-puppet-init
</code></pre></div>
<p>Then continue on to the <a href="/tutorial/">tutorial!</a></p>
<p>Then continue on to the <a href="{{site.url}}/tutorial">tutorial!</a></p>
Copy link

Choose a reason for hiding this comment

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

I'd consistently use baseurl instead of including the whole domain

Suggested change
<p>Then continue on to the <a href="{{site.url}}/tutorial">tutorial!</a></p>
<p>Then continue on to the <a href="{{site.baseurl}}/tutorial">tutorial!</a></p>

Copy link
Author

Choose a reason for hiding this comment

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

its better to use site.url across these pages, otherwise it has difficulty loading in the CSS, even when Ive tried to concat the jekyll variables, the CSS still fails to loads.

This code produces the below output href="{{site.url}}/{{site.baseurl}}/assets/css/styles.css">

image

From the description of site.baseurl you would think its something we need as it mentions using this if your site is served from a sub directory, i find this slightly confusing as we are serving our documentation from ./docs but we would only need baseurl if our project site was https://puppetlabs.github.io/rspec-puppet/docs and ./docs being the subdomain but as thats not the case we do not need baseurl as its not served from subdomain

@danadoherty639
Copy link
Author

Any reason you need to use {{ site.url }} instead of {{ site.baseurl }}?

Ive tested this with baseurl but if I remember, it broke the css, I will test this out again this morning before opening the PR

@jordanbreen28 jordanbreen28 merged commit 5084326 into main Jul 10, 2024
10 checks passed
@jordanbreen28 jordanbreen28 deleted the cat_1930 branch July 10, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants