-
Notifications
You must be signed in to change notification settings - Fork 137
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
Layout: Add auto bullet linking to included files #616
Conversation
@@ -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 |
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.
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.
Glad I added that disclaimer since they're all broken. 🤦 I'll look into fixing them, probably tomorrow. |
Concept ACK. Will review when the translations are fixed! |
Used the following command and edited as necessary: vi $( grep -rl -- '- ' _includes/ )
c5ccb9a
to
04c7452
Compare
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 |
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. |
@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. |
Thanks Dave. No problem including the first commit, so I merged it as is. |
Post merge ACK. Nice workaround. |
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:
_includes/head.html
and delete the{%- seo -%}
linemake clean build
cp -a _site _new_site
git reset --hard HEAD^^
make clean build
diff -ruN _site _new_site | colordiff | less -R
Note: I didn't really check the translations.