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

Do not markdownify title - Fixes #3094 #3113

Merged
merged 6 commits into from
May 5, 2024

Conversation

lsolesen
Copy link
Contributor

@lsolesen lsolesen commented Aug 8, 2021

Title should not be markdownified. Pull request to fix gh-3094.

Summary

Context

_includes/seo.html Show resolved Hide resolved
@iBug
Copy link
Collaborator

iBug commented Aug 8, 2021

You can add Fixes #3094 to your PR description so it's automatically closed when this is merged. See https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@lsolesen lsolesen changed the title Do not markdownify title Do not markdownify title - Fixes #3094 Aug 8, 2021
@lsolesen
Copy link
Contributor Author

lsolesen commented Aug 8, 2021

@iBug But why should the vertical bar | be escaped in the seo_title?

If it needs replacing then the replace should probably take place on seo_title together with escape_once instead, as the user might use it as part of the title also.

@iBug
Copy link
Collaborator

iBug commented Aug 8, 2021

Well you're right. I couldn't find any necessity to escape the vertical bar. Maybe it was related to Markdown parsing where it would be interpreted as table (it trolled me once upon a time). Now that you've removed markdownify, this escape shouldn't be needed anymore.

@mmistakes
Copy link
Owner

The markdownify filter does serve a purpose as it normalizes titles that may use Markdown to apply emphasis or other "light" formatting.

For example check this test post on master. https://mmistakes.github.io/minimal-mistakes/markdown/markup-title-with-markup/

Screen Shot 2021-08-08 at 1 54 14 PM

The title has a word italized and bolded, and the <title> element is readable without Markdown characters appearing.
Screen Shot 2021-08-08 at 1 51 59 PM

This PR breaks that. Now the <title> element has all the Markdown syntax there. Markdown titles are probably an edge case, but the theme has had support for them for a long time and this breaks that.

Screen Shot 2021-08-08 at 1 53 32 PM

@mmistakes
Copy link
Owner

A better fix might be to apply the Markdown and replace transforms on page.title before the separator and site.title are appended. Then it won't do unintended things on the | or any other characters that serve a purpose in Markdown.

_includes/seo.html Show resolved Hide resolved
@lsolesen
Copy link
Contributor Author

Thanks for the comments @iBug and @mmistakes. Here is a new stab on solving the issue.

@mmistakes
Copy link
Owner

@lsolesen sorry for the delay on this, finally got around to testing it. Titles with markdown parse correctly now, but the "home" index.html page doesn't output a title now. There's an empty <title></title>

Screen Shot 2022-05-27 at 10 29 24 AM

Will probably want to test a paginated home index as well as there's some logic to spit out page numbers in the title element.

_includes/seo.html Show resolved Hide resolved
@@ -18,7 +18,7 @@
{% include sidebar.html %}

<article class="page h-entry" itemscope itemtype="https://schema.org/CreativeWork">
{% if page.title %}<meta itemprop="headline" content="{{ page.title | markdownify | strip_html | strip_newlines | escape_once }}">{% endif %}
{% if page.title %}<meta itemprop="headline" content="{{ page_title }}">{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid page_title as calculated in seo.html won't propagate to here.

@iBug iBug merged commit dfaac40 into mmistakes:master May 5, 2024
1 check passed
ElponeNote added a commit to ElponeNote/ElponeNote-github.io that referenced this pull request Jun 13, 2024
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.

Title should not be markdownified
3 participants