rmm5t / jquery-timeago

:clock8: The original jQuery plugin that makes it easy to support automatically updating fuzzy timestamps (e.g. "4 minutes ago").
http://timeago.yarp.com
MIT License
3.82k stars 710 forks source link

Week #279

Closed trungdq88 closed 8 years ago

trungdq88 commented 8 years ago

Added "week" feature.

rmm5t commented 8 years ago

First, thank you for the effort here. This is a fairly clean pull-request.

Minor critique: Please never bump a version number when submitting a pull-request to another's project. ...but at least you made this a separate commit.

I'm on the fence about this PR. I can't accept it as is, but some minor changes could get it there. However, accepting those changes would violate some of the underlying goals that this project set out to achieve.

Here are some reasons why I can't accept this as is:

  1. The goal of the project has and will always be to simulate the Rails time_ago_in_words helper. It doesn't deal in weeks.
  2. This potentially breaks backwards compatibility for every custom locale ever written for this project. I'm unwilling to do that.

The only way I could get around these two concerns if this PR also came with a allowWeeks options that defaulted to false.

With all that said, I'm still on the fence, because there's still always the possibility of getting the weeks behavior without any core changes and just using a custom locale that overrides the behavior of the $.timeago.settings.strings.days function.

Example (Disclaimer...untested):

$.timeago.settings.strings.days = function(days, distanceMillis) {
  var weeks = days / 7;
  return days < 7 && "%d days" ||
    days < 11 && "about a week" ||
    (weeks + " weeks");
};

In other words, we're talking about one option override over another. One (presented with a new allowWeeks options) comes with additional complications outside the default English locale.

trungdq88 commented 8 years ago

Thank you. You are absolutely right about the backward incompatible, I was not thinking very carefully about this earlier, sorry.

IMO, I don't like the idea of using a custom locate that override the default behavior of the lib. This is a very well-done library, and since your goals is to simulate the Rails time_ago_in_words, I think this PR is not suitable - out of scope. We can close this.

P/S: Please never bump a version number when submitting a pull-request to another's project Thank you for this too.