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

[feature] Enable relative time for multiple seconds, request #2558 #3738

Closed
wants to merge 1 commit into from

Conversation

pndewit
Copy link
Contributor

@pndewit pndewit commented Jan 31, 2017

This PR adds an optional threshold (ss) for a few seconds to %d seconds. It is built so that it will never display %d seconds UNLESS the user manually sets the ss threshold. Setting the ss threshold once will separate the default value (which is the value in the s threshold minus 1).

As mentioned in #2558, there is no translation for: %d seconds. I could add this for each locale using the seconds word that is already present in each locale for the s threshold (e.g. a few **seconds**), but I don't know if this would work for all locales. So then this would require a review from a native speaker for each locale. Probably not gonna happen.

#2182: I hope by not actively using this ss threshold by default (afaik this is backwards compatible), this PR still has a chance.

Let me know what you think :)

@pndewit
Copy link
Contributor Author

pndewit commented Feb 21, 2017

@ichernev is your comment in #2182 still valid?

@@ -60,6 +63,11 @@ export function getSetRelativeTimeThreshold (threshold, limit) {
if (limit === undefined) {
return thresholds[threshold];
}
if (threshold === 's' && !secondsThresholdChanged) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit tricky. I'd drop the variable, always set ss to limit - 1, so if you need to use it you need to set ss last (alone or after s, which makes sense).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, doing the cleanup and the merge! :)

@ichernev
Copy link
Contributor

ichernev commented Mar 5, 2017

@pndewit its valid, just clean it a bit as requested and it'll be merged.

@ichernev ichernev added this to the 2.18.0 milestone Mar 5, 2017
@ichernev ichernev added the todo label Mar 5, 2017
@ichernev
Copy link
Contributor

ichernev commented Mar 5, 2017

Also make a corresponding PR to the docs repo: https://github.com/moment/momentjs.com

@ichernev
Copy link
Contributor

Merged in ed1fc74

@ichernev ichernev changed the title humanize) Adds optional threshold for a few seconds to %d seconds: #2558 [feature] Enable relative time for multiple seconds, request #2558 Mar 11, 2017
@ichernev ichernev closed this Mar 11, 2017
ichernev added a commit that referenced this pull request Mar 11, 2017
[feature] Enable relative time for multiple seconds, request #2558
@pndewit
Copy link
Contributor Author

pndewit commented Mar 13, 2017

Docs PR created: moment/momentjs.com#404

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

Successfully merging this pull request may close these issues.

None yet

2 participants