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

Support DATE_FANCINESS and updated in base and base-jinja #3135

Merged
merged 5 commits into from Aug 10, 2018
Merged

Conversation

@gwax
Copy link
Contributor

@gwax gwax commented Aug 9, 2018

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

This PR changes two things relating to base/base-jinja handling of datetime in posts:

  1. addition of "updated" time, if specified in the post metadata (output is unchanged if "updated" no specified)
  2. DATE_FANCINESS is supported for published and updated dates (output is unchanged if DATE_FANCINESS=0
@gwax gwax requested a review from Kwpolska Aug 9, 2018
@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 9, 2018

PR fails on baseline check (because of changed content) but is otherwise passing.

I have tested the changes locally against my own nikola setup.

Copy link
Member

@Kwpolska Kwpolska left a comment

Pretty good, but we need to make this translatable.

<a href="{{ post.permalink() }}" rel="bookmark">
<time class="published dt-published" datetime="{{ post.formatted_date('webiso') }}" itemprop="datePublished" title="{{ post.formatted_date(date_format)|e }}">{{ post.formatted_date(date_format)|e }}</time>
{% if post.updated and post.updated != post.date %}
<span class="updated"> updated </span>
Copy link
Member

@Kwpolska Kwpolska Aug 9, 2018

Needs translation support, preferably in the style of {published_date} <span class="updated">(updated {updated_date})</span> because languages might have special requirements. (Translating at least the word updated is mandatory.)

Copy link
Contributor Author

@gwax gwax Aug 10, 2018

I'm not quite sure how to have the messages update span across the <time>...</time> section but I can certain create a message around "updated".

Copy link
Member

@Kwpolska Kwpolska Aug 10, 2018

Come to think about it, it won’t be easy to do. Let’s scrap that.

CHANGES.txt Outdated
@@ -8,6 +8,14 @@ Features

* Don’t generate gallery index if the destination directory is
site root and it would conflict with blog index (Issue #3133)
* ``base`` and ``base-jinja`` templates now support updated
Copy link
Member

@Kwpolska Kwpolska Aug 9, 2018

Please copy the change to bootblog4/index.tmpl and change the line to

* All built-in themes now support ``updated``

@gwax gwax force-pushed the base_fancytime branch from 64ce2b4 to 03d5846 Aug 10, 2018
@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 10, 2018

Done.

I know that updating the messages/* files by hand is not the right way to do things and liable to break the next time someone updates the translations but I could not figure out how to request access to Transifex as an English speaker or otherwise add a new message type.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Aug 10, 2018

Regarding translations, adding them to Transifex requires admin rights, I’ll take care of that.

Copy link
Member

@Kwpolska Kwpolska left a comment

LGTM. I’ll change it to say (updated XX) because that looks better to me — hope you don’t mind that.

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska Kwpolska merged commit a4da141 into master Aug 10, 2018
0 of 5 checks passed
@Kwpolska Kwpolska deleted the base_fancytime branch Aug 10, 2018
@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 10, 2018

@Kwpolska changing to (updated XX) is totally fine in my book but I believe you forgot to run scripts/jingify.py because the jinja versions do not appear to have been updated.

Thanks.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Aug 10, 2018

Yeah, I forgot. Fixed on master.

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

Successfully merging this pull request may close these issues.

None yet

2 participants