Skip to content

Layout: Add auto bullet linking to included files #616

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

Merged

Conversation

harding
Copy link
Collaborator

@harding harding commented Jul 29, 2021

Closes #604

Apparently it's a known limitation of Jekyll that Liquid template processing doesn't work on {% include %} files, which is why the automatic bullet linking isn't working. This PR adds a {% auto_anchor %}...{% endauto_anchor %} block filter that we can put in included files in order to run the anchoring code manually (including calling Liquid).

To test this by diffing the site, I found it necessary to temporarily disable the SEO plugin which outputs a non-deterministic entry in the header of every page. There's already a bug for that upstream. (I don't know why this just became a problem now; I've diffed plenty of times before without problems.) Temporarily disabling the SEO plugin required adding the template from Jekyll Minima to our repository, which is the first commit in this PR. (To be clear, this PR only adds the ability to disable the plugin, it doesn't actually change anything.)

Diff instructions are:

  • Checkout this PR
  • Edit _includes/head.html and delete the {%- seo -%} line
  • Build the site: make clean build
  • Backup the output: cp -a _site _new_site
  • Drop the two real commits: git reset --hard HEAD^^
  • Make sure the SEO tag is still deleted
  • Build the site again: make clean build
  • Diff the output: diff -ruN _site _new_site | colordiff | less -R

Note: I didn't really check the translations.

@@ -1,4 +1,4 @@
As of this writing, here are what we think are some of the most
{% auto_anchor %}As of this writing, here are what we think are some of the most
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: the lack of newline here is deliberate. I did a weird thing when adding this to Newsletter 60 and so we can't have a newline here.

@harding
Copy link
Collaborator Author

harding commented Jul 29, 2021

Note: I didn't really check the translations.

Glad I added that disclaimer since they're all broken. 🤦‍ I'll look into fixing them, probably tomorrow.

@jnewbery
Copy link
Contributor

Concept ACK. Will review when the translations are fixed!

Used the following command and edited as necessary:

  vi $( grep -rl -- '- ' _includes/ )
@harding harding force-pushed the 2021-07-fix-auto-linking-included branch from c5ccb9a to 04c7452 Compare July 30, 2021 13:34
@harding
Copy link
Collaborator Author

harding commented Jul 30, 2021

The problem with the translations was the limitations with Kramdown previously described in #349 (comment) , so in this PR I just dropped the additional processing of the Japanese files. If the bullet titles in translations are updated to add some unique latin characters to each bullet, it should be safe to add the {% auto_anchor %}...{% endauto_anchor %} back.

@jnewbery
Copy link
Contributor

jnewbery commented Aug 2, 2021

ACK 04c7452

Do you want the first commit (Templates: add head.html from jekyll-minima project (MIT license)) to be merged with this PR, or should it be removed first? It's not clear to me whether it was intended to be just temporary for testing the PR.

@harding
Copy link
Collaborator Author

harding commented Aug 2, 2021

@jnewbery Unless you or anyone else objects, I'd like that first commit merged as it'll make future diffing of the site build contents easier. However, it isn't needed for this PR and so can be dropped without issue.

@jnewbery jnewbery merged commit 699b1b0 into bitcoinops:master Aug 2, 2021
@jnewbery
Copy link
Contributor

jnewbery commented Aug 2, 2021

Thanks Dave. No problem including the first commit, so I merged it as is.

@bitschmidty
Copy link
Contributor

Post merge ACK. Nice workaround.

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.

Automatic bullet linking doesn't work for included files
3 participants