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

Use Jekyll for Playbook #355

Closed
wants to merge 45 commits into from
Closed

Use Jekyll for Playbook #355

wants to merge 45 commits into from

Conversation

pezholio
Copy link
Contributor

@pezholio pezholio commented Jun 3, 2020

Fixes #349

Until now, we've used Docsify to power the Playbook, which has worked OK, but it doesn't work at all with JavaScript turned off. We wouldn't accept this in client projects, so it makes sense for the Playbook to meet the same standards.

I've tweaked the Playbook to use Jekyll to build the site, as well as using Webpack to build the limited Javascript functionality. Unfortunately, as the Playbook is a static site, the search won't work without Javascript, but I think this is a fair trade-off.

I've got this branch deploying to Netlify, as it gives us more flexibility in how we deploy the site. Assuming we're OK with this, it'll need a bit more configuration for us to move it across.

You can see the branch deployed the Netlify here - https://competent-bhabha-662efd.netlify.app/

webpack.config.js Show resolved Hide resolved
_plugins/navigation.rb Show resolved Hide resolved
_plugins/navigation.rb Show resolved Hide resolved
_plugins/navigation.rb Outdated Show resolved Hide resolved
_plugins/navigation.rb Show resolved Hide resolved
_plugins/navigation.rb Show resolved Hide resolved
_plugins/navigation.rb Show resolved Hide resolved
_javascripts/src/lib/sidebar_toggle.js Outdated Show resolved Hide resolved
_javascripts/src/lib/sidebar_toggle.js Outdated Show resolved Hide resolved
_javascripts/src/lib/sidebar_toggle.js Outdated Show resolved Hide resolved
@pezholio
Copy link
Contributor Author

pezholio commented Jun 3, 2020

Copy link
Contributor

@erbridge erbridge left a comment

Choose a reason for hiding this comment

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

Not a complete review, but let's also install and use Prettier and ESLint for JavaScript linitng and formatting.

_javascripts/src/lib/search.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
@pezholio pezholio force-pushed the jekyllise-playbook branch from 1d5e634 to 24e1f28 Compare June 3, 2020 10:39
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
script/test Outdated Show resolved Hide resolved
@CristinaRO
Copy link
Contributor

Running script/bootstrap locally, with a pre-existing installation, it goes well until the last install:

Installing Vale
# github.com/errata-ai/vale/core
../../go/src/github.com/errata-ai/vale/core/util.go:231:8: undefined: strings.ToValidUTF8

@CristinaRO
Copy link
Contributor

The scrolling within the file, when clicking an inner anchor link, is very slow and makes me dizzy.

Is it slower because it's on my local, or could it be caused by the change of framework?

@pezholio
Copy link
Contributor Author

pezholio commented Jun 3, 2020

@CristinaRO It's caused by the call to scrollIntoView here https://github.com/dxw/playbook/blob/jekyllise-playbook/_javascripts/src/lib/sidebar_navigation.js#L7. I'll have a look if it can be made smoother 🙂

_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
@jacksonj04
Copy link
Member

In line with running Prettier on JS, we could probably do with running it over all the Markdown as well to ensure it's clean and pretty (as well as throw a test in which stops people checking in messy markdown as well).

webpack.config.js Show resolved Hide resolved
_spec/sidebar_toggle.test.js Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
_spec/search.test.js Outdated Show resolved Hide resolved
_javascripts/src/lib/sidebar_toggle.js Outdated Show resolved Hide resolved
Gemfile Show resolved Hide resolved
Gemfile Show resolved Hide resolved
Gemfile Show resolved Hide resolved
Gemfile Show resolved Hide resolved
Gemfile Show resolved Hide resolved
@pezholio
Copy link
Contributor Author

pezholio commented Jun 3, 2020

We do use Hound for linting the Markdown, which makes the visibility of problems easier for non-devs

@pezholio
Copy link
Contributor Author

pezholio commented Jun 3, 2020

(Although, at the moment, it seems to be linting the JS and Ruby too, making a helluva lot of noise!)

@pezholio pezholio force-pushed the jekyllise-playbook branch from 7bafd3f to edcbe81 Compare June 3, 2020 14:21
@pezholio
Copy link
Contributor Author

pezholio commented Jun 3, 2020

Running script/bootstrap locally, with a pre-existing installation, it goes well until the last install:

Installing Vale
# github.com/errata-ai/vale/core
../../go/src/github.com/errata-ai/vale/core/util.go:231:8: undefined: strings.ToValidUTF8

@CristinaRO What version of Go are you using? (go version). I could replicate this in CI until I changed the Go version to 1.14

@pezholio pezholio force-pushed the jekyllise-playbook branch from f5fdfc0 to c0b3c67 Compare June 3, 2020 15:41
@CristinaRO
Copy link
Contributor

@CristinaRO What version of Go are you using? (go version). I could replicate this in CI until I changed the Go version to 1.14

go version go1.12 darwin/amd64

Shouldn't this be part of the bootstrap standard operating procedure though? Or at least specified in the readme / dependencies.

@pezholio
Copy link
Contributor Author

pezholio commented Jun 3, 2020

Yep, I’ll add that now I know that’s the issue 👍🏻

@erbridge
Copy link
Contributor

erbridge commented Jun 3, 2020

@pezholio pezholio force-pushed the jekyllise-playbook branch 5 times, most recently from 5149675 to df4fe54 Compare June 5, 2020 08:59
@pezholio
Copy link
Contributor Author

pezholio commented Jun 5, 2020

Copy link
Contributor

@erbridge erbridge left a comment

Choose a reason for hiding this comment

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

I'm happy to help make some of the changes I'm suggesting. Let me know.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
.prettierrc Outdated
{
"trailingComma": "es5",
"singleQuote": true,
"semi": false
Copy link
Contributor

Choose a reason for hiding this comment

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

👎

We're running through Babel, so let's use modern standards and options. And JavaScript has strange behaviour in some cases around semicolons, so let's not turn them off. Defaults are best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, will do. I fully admit to not really knowing what the "best" way of doing these kind of things are, so appreciate the steer 👍

README.md Outdated Show resolved Hide resolved
_spec/sidebar_navigation.test.js Outdated Show resolved Hide resolved
@@ -1,3 +1,5 @@
---
---
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably be using the metadata blocks to add things like page titles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I shied away from this was I didn't want to add any additional workload for non-devs who are contributing to the Playbook. This is a very loosely-held opinion though

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an upgrade. If they don't do it, it will have the default title and all will be well :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, this is done now 👍

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@pezholio pezholio force-pushed the jekyllise-playbook branch from c113415 to eddf518 Compare June 5, 2020 12:24
pezholio and others added 22 commits June 10, 2020 14:36
# Conflicts:
#	package-lock.json
#	package.json
This fixes most of the accessilbility issues as outlined in the automated
check in #350
Also, use the `*-version` files to specify the versions to use in Github actions
This isn't always available when specifying the ruby version in Actions
Also, move the dmark image out of the `build` folder
Fixes the runtime warning
`Liquid Warning: Liquid syntax error (line 70): Expected end_of_string but found id in "{{A SPECIFIC PAGE}}" in guides/plugin-advisories.md`.

This did not come up during linting.

The text “a specific page” was not appearing, because it was being treated as a placeholder.

Use square brackets instead or curly, for consistency with the other example advisories, and capitalise similarly to the other advisories.

NB: The existing capitalisation goes against our style recommendations, but it is consistent throughout the plugin advisories guide.
Now the top-level headings are handled in the frontmatter, the way the
navigation is handled needs tweaking slightly
We had a link that didn't go anywhere, and now we have the frontmatter, it's
easier to get the title of each page to autogenerate a list
@pezholio pezholio force-pushed the jekyllise-playbook branch from 354520a to 3276bde Compare June 10, 2020 13:36
.eslintrc.js Show resolved Hide resolved
.babelrc Show resolved Hide resolved
.babelrc Show resolved Hide resolved
.babelrc Show resolved Hide resolved
README.md Show resolved Hide resolved
script/bootstrap Show resolved Hide resolved
script/cibuild Show resolved Hide resolved
script/cibuild Show resolved Hide resolved
script/test Show resolved Hide resolved
script/update Show resolved Hide resolved
@erbridge erbridge self-assigned this Jun 11, 2020
@erbridge erbridge mentioned this pull request Jun 12, 2020
3 tasks
@erbridge
Copy link
Contributor

Closing in favour of #365.

@erbridge erbridge closed this Jun 12, 2020
@erbridge erbridge deleted the jekyllise-playbook branch March 16, 2022 09:30
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.

Playbook doesn't work with JavaScript disabled
6 participants