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

Fix accordions to support Jekyll 3.9.0 #509

Merged
merged 4 commits into from
Nov 17, 2020
Merged

Conversation

arthurattwell
Copy link
Member

This is a primarily change to accordion.js, the script that turns long pages into collapsible sections for easier reading. It fixes an issue on GitHub Pages where maths was not being included in accordion sections, and was appearing out of position.

Here's the background, for posterity.

  • Till now, we've been using kramdown v1, which is standard with Jekyll up to v3.8. kramdown v1 wraps MathJax in <script> tags. That means that they are script elements in the source HTML, and can be targeted as such (before the MathJax JS renders them as maths). When our accordions.js Javascript creates the accordion sections, it finds the section headings (usually the h2s), creates a <section> for each one, and then moves all the elements between section headings into the relevant <section>. Note that I said it moves all elements into the accordion <section>.
  • Meanwhile, kramdown has moved on to v2, with some breaking changes. One of those is that MathJax is no longer wrapped in <script> tags. This is because the latest MathJax v3 doesn't support them. Instead, kramdown just leaves the math in the HTML wrapped in \[ ... \] characters. This means that math blocks are no longer elements. They are, in HTML terms, text nodes. The MathJax JS still works to render maths. But when our accordion script tries to move a section's elements into the right <section>, it doesn't move the text nodes. They get left behind, lying around in the DOM after the correct <section>, and before the next one. They are not even 'in' the accordion. That's what this PR fixes.
  • After a recent discovery of a vulnerability in kramdown v1, the Jekyll team released v3.9, which uses kramdown v2. Yikes! I think this is rash, or at best an unavoidable PITA: it introduces kramdown v2's backwards-incompatible breaking changes in a Jekyll minor-version update. There's a note about this change here.
  • In EBT projects, we specify the version of Jekyll to use in our Gemfile. And till now we've only ever used up to v3.8.6, which uses kramdown v1. And we could stick with that to avoid Jekyll 3.9 and kramdown's breaking changes. However, unlike hosts like Netlify and our own local builds, GitHub Pages ignores our Gemfiles, and strictly uses only its own supported gems.

I don't know yet what the effect of this kramdown change would be on outputs like print and epub. I've done some testing of PDF and epub outputs and haven't noticed any issues so far with the Samples book in the EBT.

I suspect maintaining support for/alignment with GitHub Pages in the EBT will get harder if GH Pages maintains its strict versioning, where everyone has to use the same version of Jekyll – especially if GH Pages moves to Jekyll v4 at some stage. This would be a great pity, because 'out of the box' support for GH Pages is a great feature – every copy of the EBT on GitHub is already a working site. That said, services like Netlify have made it much easier to host for free with more flexibility since I first built the EBT with GH Pages in mind.

arthurattwell added a commit that referenced this pull request Nov 6, 2020
This will be resolved in #509. For now, if we try to use
kramdown v2 dependencies with Jekyll 3.8 or lower, bundler
will error because kramdown-parser-gfm depends on kramdown v2.
This error breaks builds on Netlify. GH Pages will build
either way because it ignore the Gemfile and currently uses
Jekyll 3.9 anyway.
Gemfile Outdated
# GitHub Pages recommends using the github-pages gem instead,
# but this injects unwanted files: https://github.com/github/pages-gem/issues/482
# GitHub Pages recommends using the github-pages gem instead.
# This caused issues in the past: https://github.com/github/pages-gem/issues/482
# Uncomment the following line (and comment out the Jekyll version above)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the default behaviour is changing here, I think this line should read "Comment out the following line (and uncomment the Jekyll version above)" or something to that effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

@LouiseSteward Ah, nice catch, thanks. I've fixed up these code comments in c31e046.

@arthurattwell arthurattwell merged commit 5061d74 into master Nov 17, 2020
@arthurattwell arthurattwell deleted the support-jekyll390 branch November 17, 2020 10:34
@arthurattwell
Copy link
Member Author

Some further notes for posterity.

This PR implements two things.

First, it's the fix for an issue we spotted on a client project, where maths wasn't being included in the accordion. That was described in my earlier post in this thread.

Second, it uses the github-pages gem by default in the Gemfile. This is quite a big decision, so worth some reflection. I'm only 60% sure it's the right decision.

Till now, we've specified a Jekyll version (e.g. '~> 3.8'), and hoped that the Jekyll version used by GitHub Pages is compatible.

  • Pro: full control over the Jekyll version used is in the hands of the repo owner.
  • Con: GitHub Pages builds can look different from local and other builds, because GHP ignores the Gemfile and forces its own set of gems.

In this PR, I've enabled the github-pages gem in the Gemfile, and disabled specifying the Jekyll version. The github-pages gem sets the Jekyll version for us, and adds some other gems to ensure that the site build matches what GitHub Pages builds.

  • Pro: the repo will always use exactly the Jekyll version supported by GHP. This ensures that all builds (local, GHP, Netlify, etc.) will look the same.
  • Con: if the github-pages gem (which matches GHP's current setup) ever changes the version of Jekyll to something that breaks a site, the site breaks everywhere, not only on GHP. E.g. if GitHub decides to switch to Jekyll 4. (A change that major would be insane because it would break sites everywhere, but people do insane things, and forcing a single Jekyll version for all GHP users is already a little nuts.)

Of course, we can always change the Gemfile to suit our projects. The main issue is what we think is best for our future selves, colleagues and others using the template.

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.

None yet

2 participants