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

Update Reading time helper #9569

Closed
JohnONolan opened this issue Apr 16, 2018 · 5 comments · Fixed by #9573
Closed

Update Reading time helper #9569

JohnONolan opened this issue Apr 16, 2018 · 5 comments · Fixed by #9573
Labels
bug [triage] something behaving unexpectedly good first issue [triage] Start here if you've never contributed before. server / core Issues relating to the server or core of Ghost

Comments

@JohnONolan
Copy link
Member

JohnONolan commented Apr 16, 2018

image

We don't really want to distinguish between posts which take 1 minute to read and posts which take less than 1 minute to read. The minimum reading time for a post should simply be 1 minute.

https://github.com/TryGhost/Ghost/blob/master/core/server/helpers/reading_time.js

This needs a very small refactor to make that change.

@JohnONolan JohnONolan added bug [triage] something behaving unexpectedly good first issue [triage] Start here if you've never contributed before. labels Apr 16, 2018
@rkalis
Copy link
Contributor

rkalis commented Apr 16, 2018

I've implemented some stuff for the reading time helper before, I'll pick this up tonight.

@kirrg001 kirrg001 added server / core Issues relating to the server or core of Ghost optimisation labels Apr 16, 2018
@rkalis
Copy link
Contributor

rkalis commented Apr 16, 2018

We currently have the following usage for reading time i18n:

{{reading_time seconds=(t "< 1 min read") minute=(t "1 min read") minutes=(t "% min read")}}

If we make the min read time 1 minute, the seconds option becomes obsolete. I would say we still want to differentiate between singular and plural minutes for i18n, do we want to keep their names as 'minute' and 'minutes' or rename them to something like 'singular' and 'plural'? I'm guessing for backwards compatibility it's better to leave the names as-is, but do we just altogether remove the 'seconds' option?

@aileen
Copy link
Member

aileen commented Apr 17, 2018

but do we just altogether remove the 'seconds' option?

Exactly my question as well.
cc @kirrg001

@kirrg001
Copy link
Contributor

I'm guessing for backwards compatibility it's better to leave the names as-is

Yeah please keep naming as is. We don't want to break themes.

but do we just altogether remove the 'seconds' option?

Yeah. It only changes a tiny detail of the reading time helper's output - probably comparable to what we did with the meta title in the past, see. People can still pass seconds, but it won't modify the output anymore.

We should shout on https://themes.ghost.org, so people can update their theme locales.

aileen pushed a commit that referenced this issue Apr 18, 2018
closes #9569

- Removed the `<1 min read` time clause, effectively making `1 min read` the minimum reading time
- Removed the `seconds` option for i18n strings, which contained the less than one minute display string
- Kept the other i18n string options the same
- Amended and improved tests for new functionality
@aileen
Copy link
Member

aileen commented Apr 18, 2018

@kirrg001 The docs on https://themes.ghost.org are updated, we just need to remember to add a shout out on the changelog for the next release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug [triage] something behaving unexpectedly good first issue [triage] Start here if you've never contributed before. server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants