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

Taxonomy language link left-overs #2844

Merged
merged 19 commits into from Jun 22, 2017
Merged

Taxonomy language link left-overs #2844

merged 19 commits into from Jun 22, 2017

Conversation

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Jun 16, 2017

This completes some things missing in #2778:

  • Adding some missing hreflangs
  • Get rid of also_create_classifications_from_other_languages for tags and categories, and marking this option as deprecated
  • Refactored feed link creation to create correct translated feed links, also simplified code a bit
  • Got rid of rss_link overriding in tags, categories and some more taxonomies, which prevented correct translated RSS feed links
  • Now really all generated .html pages should have links to main RSS and Atom feeds
  • Fixed bug: language was not passed for title and link generation when list.tmpl was used for a classification page

(fixes #993; fixes #2785)

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Jun 16, 2017

(The only changes in the baseline are added hreflangs.)

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

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

Nuke the setting altogether. I doubt there are any third-party taxonomy plugins that depend on it.

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

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>

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

That’s not going to pass jinjification.

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

That reminds me I forgot to run jinjify...

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

I modified jinjify in a340ffe so it can handle this.

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

(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.)

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

A bit more code duplication is better than unreadable one-liners.

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

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.

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

That might look better. _proper screams “this is an ugly hack”

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

The _proper was to avoid having a nested if in a one-liner :)

I've now refactored it to avoid one-line macros in 00b2a80 and 1bb186b.

## 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)}

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

Jinja probably won’t like this inline if either.

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

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>

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

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)}

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

Is kind present in /index.html?

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

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)}

This comment has been minimized.

@Kwpolska

Kwpolska Jun 16, 2017
Member

We shouldn’t use maybe-defined variables in every file. We should avoid undefined variables if possible.

This comment has been minimized.

@felixfontein

felixfontein Jun 16, 2017
Author Contributor

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.

@Kwpolska Kwpolska merged commit f9f3c54 into master Jun 22, 2017
3 of 5 checks passed
3 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Kwpolska Kwpolska deleted the taxonomy-lang-refs-2 branch Jun 22, 2017
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Jun 22, 2017

Thanks for doing this!

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Jun 23, 2017

Thanks for reviewing and merging! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants