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
Taxonomy language link left-overs #2844
Conversation
…nd categories, and deprecating the setting (#2785).
…author post lists).
(The only changes in the baseline are added |
CHANGES.txt
Outdated
@@ -5,11 +5,17 @@ Features | |||
-------- | |||
|
|||
* Use ``PRETTY_URLS`` by default on all sites (Issue #1838) | |||
* `also_create_classifications_from_other_languages` is deprecated |
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.
Nuke the setting altogether. I doubt there are any third-party taxonomy plugins that depend on it.
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.
Done.
## Handles both feeds and translations | ||
<%def name="head(classification=None)"> | ||
% if rss_link: | ||
<%def name="_append_language(language)">${ " (" + language + ")" if len(translations) > 1 else "" }</%def> |
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.
That’s not going to pass jinjification.
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.
That reminds me I forgot to run jinjify...
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.
I modified jinjify in a340ffe so it can handle this.
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.
(FYI: I need this "the whole macro in one line" because it seems to be impossible to get rid of the generated whitespace in Mako otherwise. And if these macros generate superfluous whitespace, this screws up the generated link texts. In Jinja2, this could be done better with whitespace control.)
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.
A bit more code duplication is better than unreadable one-liners.
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.
If it would be only a bit more, I'd agree. But here it will be a lot.
Except if I add a function for generating a <link>
or <a>
which contains all the cases in there and has some more additional variables. I don't know if that's easier to read.
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.
That might look better. _proper
screams “this is an ugly hack”
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.
## Handles both feeds and translations | ||
<%def name="head(classification=None, kind='index', feeds=True, other=True, rss_override=True, has_no_feeds=False)"> | ||
% if feeds and not has_no_feeds: | ||
${_head_rss(classification, 'index' if (kind == 'archive' and rss_override) else kind, rss_override)} |
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.
Jinja probably won’t like this inline if either.
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.
That survived jinjification (as opposed to the above) and should work fine in jinja.
## Handles both feeds and translations | ||
<%def name="head(classification=None)"> | ||
% if rss_link: | ||
<%def name="_append_language(language)">${ " (" + language + ")" if len(translations) > 1 else "" }</%def> |
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.
A bit more code duplication is better than unreadable one-liners.
@@ -16,7 +16,7 @@ | |||
|
|||
<%block name="content"> | |||
<%block name="content_header"> | |||
${feeds_translations.translation_link()} | |||
${feeds_translations.translation_link(kind)} |
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.
Is kind
present in /index.html?
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.
Yes, /index.html
is generated by the index
taxonomy.
@@ -42,7 +42,7 @@ lang="${lang}"> | |||
% if meta_generator_tag: | |||
<meta name="generator" content="Nikola (getnikola.com)"> | |||
% endif | |||
${feeds_translations.head()} | |||
${feeds_translations.head(classification=None, kind=kind, other=False)} |
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.
We shouldn’t use maybe-defined variables in every file. We should avoid undefined variables if possible.
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.
There was actually no need for using kind
, since classification
is set to None
. I've now set kind
to 'index'
, which generates the same result.
Thanks for doing this! |
Thanks for reviewing and merging! :) |
This completes some things missing in #2778:
hreflang
salso_create_classifications_from_other_languages
for tags and categories, and marking this option as deprecatedrss_link
overriding in tags, categories and some more taxonomies, which prevented correct translated RSS feed linkslist.tmpl
was used for a classification page(fixes #993; fixes #2785)