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

Variant of Typogrify filter without ampersand #3405

Closed
wants to merge 4 commits into from

Conversation

ncdulo
Copy link

@ncdulo ncdulo commented May 2, 2020

Pull Request Checklist

  • I’ve read the guidelines for contributing.
  • I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
  • I tested my changes.

Description

In reference to issue #3404 this pull request adds a variation of the base Typogrify filter without the ampersand filtering. The only change made to the standard filters.typogrify is removal of the typo.amp() call. This has been tested on my own site using Nikola 8.0.4, and from a test site using Nikola master. The work for this PR is based off of master.

This pull fixes #3404

ncdulo added 4 commits May 2, 2020 12:30
Add a variant of the Typogrify filter (`typogrify_sans_amp`) without any ampersand modification to the output. When a page or post title contains ampersands, Typogrify wraps them in `<span>` tags. This causes the `<span>` to potentially appear in your `<title>` blocks, unless they are stripped before the Typogrify filter runs.

This variant will skip Typogrify ampersand filtering from running, leaving your `<title>` intact. Note that if you rely on the `<span>` tag to style your ampersands, this filter will break that as the `<span>` will no longer be included into your final output.
Write entry regarding fix of issue getnikola#3404
Include GitHub profile link for author of fix for issue getnikola#3404
Include mention of `typogrify_sans_amp` filter.
@ncdulo ncdulo changed the title Ncdulo typogrify sans amp Variant of Typogrify filter without ampersand May 2, 2020
@Kwpolska
Copy link
Member

Kwpolska commented May 5, 2020

Perhaps the filters could just be fixed to ignore some tags?

We could take the typogrify function, change the applyfilters thing, and fix this (and possibly other bugs) without an extra filter and without having still more broken things: https://github.com/mintchaos/typogrify/blob/master/typogrify/filters.py#L348

@ncdulo
Copy link
Author

ncdulo commented May 5, 2020

That sounds like a great solution. I was thinking something similar the other day. More of a way to pass a set of parameters to the typogrify function which define a selection of filters to run. That way a user would be pick exactly the set of Typogrify filters to run. I did not further pursue that solution because I was unsure of how those parameters might be passed around, and just haven't really had the energy to dig too deeply into it.

I'm at work so I can't take a deep look at the code right now, but will check it out later on. From what you mention, it looks like yours would be a better fitting solution. I'm all for reducing the duplication, and preventing other issues from arising.

@ncdulo
Copy link
Author

ncdulo commented Jul 14, 2020

With this pull request being two months old, and life situations pulling me away from working towards a more appropriate fix, I don't foresee myself able to commit to any further work on this in the near future. I'd rather not leave this hanging, open on the PR board if no work is being done. If someone wants to pick this up and handle the issue more properly than the solution I came up with that would be great. I'd really like to see the original issue fixed in Nikola. Otherwise it may be time to close this one.

Thanks to the project developers for taking the time to offer suggestions towards a better fix here, and through other mediums. I'll continue maintaining the original fix implemented in my private (for unrelated reasons) Nikola source repo that powers my website.

@Kwpolska
Copy link
Member

I implemented the full fix in pull request #3447.

@Kwpolska Kwpolska closed this Jul 29, 2020
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.

Typogrify filter places <span> into certain page <title> blocks
2 participants