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

44 to 44 seconds #4086

Closed
Falci opened this issue Jul 26, 2017 · 3 comments · Fixed by moment/momentjs.com#505
Closed

44 to 44 seconds #4086

Falci opened this issue Jul 26, 2017 · 3 comments · Fixed by moment/momentjs.com#505

Comments

@Falci
Copy link

Falci commented Jul 26, 2017

Description of the Issue and Steps to Reproduce:

This documentation is not clear about the ss key: 44 to 44 seconds.

What does it mean? Is it a special case that affects only exactly 44 seconds? Won't 44 seconds be covered by the s section? (0 to 44 seconds).

Environment:
Documentation section from momentjs.com

@icambron
Copy link
Member

icambron commented Aug 1, 2017

Yeah, I think that is what it's suppose to be saying. That's a) a little weird, and b) doesn't actually work. This code seems a little buggy:

var thresholds = {
    ss: 44,         // a few seconds to seconds
    s : 45,         // seconds to minute
    m : 45,         // minutes to hour
    h : 22,         // hours to day
    d : 26,         // days to month
    M : 11          // months to year
};

function relativeTime$1 (posNegDuration, withoutSuffix, locale) {
 
  //snip

    var a = seconds <= thresholds.ss && ['s', seconds]  ||
            seconds < thresholds.s   && ['ss', seconds] ||
            minutes <= 1             && ['m']           ||
            minutes < thresholds.m   && ['mm', minutes] ||
            hours   <= 1             && ['h']           ||
            hours   < thresholds.h   && ['hh', hours]   ||
            days    <= 1             && ['d']           ||
            days    < thresholds.d   && ['dd', days]    ||
            months  <= 1             && ['M']           ||
            months  < thresholds.M   && ['MM', months]  ||
            years   <= 1             && ['y']           || ['yy', years];

    //snip
}

It checks <= on the 44 and < on the 45 so the SS case never hits. I suspect that we meant ss to not be in play at all with the default thresholds (which makes sense because we added it recently and reverse compatibility and all that), but even the thresholds seem flipped between s and ss? Regardless, it's clearly poorly documented. Would accept a patch to the docs and if I'm right about the logic being a bit wonky, then a patch for the code too.

See #3738

@Falci
Copy link
Author

Falci commented Aug 1, 2017

IMO, the fix could be

  1. remove the ss option from docs and source code; or
  2. reduce the s range. Something like 20 seconds, but this is a random value.

@marwahaha
Copy link
Member

Copied from the PR @icambron linked:

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

Does this make sense? @Falci - Would love a contribution to our docs at https://github.com/moment/momentjs.com/

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

Successfully merging a pull request may close this issue.

3 participants