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

Making CakeTime display 'about X ago' when time passed is lower than acc... #1511

Merged
merged 1 commit into from Aug 12, 2013
Merged

Making CakeTime display 'about X ago' when time passed is lower than acc... #1511

merged 1 commit into from Aug 12, 2013

Conversation

ankr
Copy link
Contributor

@ankr ankr commented Aug 12, 2013

...uracy

This would produces an incorrect value:
CakeTime::timeAgoInWords(strtotime('-58 minutes'), array('accuracy' => 'hour'));

Old result: " ago" (notice empty space)
New result: "about an hour ago"

@lorenzo
Copy link
Member

lorenzo commented Aug 12, 2013

👍 for $fWords

return __d('cake', '%s ago', $relativeDate);
if ($relativeDate) {
return __d('cake', '%s ago', $relativeDate);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

you can spare the else statement as the if has a return inside

Copy link
Member

Choose a reason for hiding this comment

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

@lorenzo was faster :)

@dereuromark
Copy link
Member

You should add a second test with the different direction (so that $backwards is false).

@ankr
Copy link
Contributor Author

ankr commented Aug 12, 2013

@dereuromark It now says "in about an hour" for future times where timeTo is less than accuracy.

@lorenzo
Copy link
Member

lorenzo commented Aug 12, 2013

This makes sense to me 👍

@dereuromark
Copy link
Member

@ankr Squash it, please. Then it looks pretty much good to go.

return __d('cake', 'about an hour ago');
}

return __d('cake', 'about a %s ago', $fWord);
Copy link
Member

Choose a reason for hiding this comment

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

Is this effectively untranslatable? Is it going to produce:

hace un **year**

?

Copy link
Member

Choose a reason for hiding this comment

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

Correction, this will make it impossible to translate correctly (ignoring whether $fWord is translated or not) in some/many languages, using spanish:

Hace aproximadamente un segundo
Hace aproximadamente un minuto
Hace aproximadamente una hora
Hace aproximadamente un dia
Hace aproximadamente un**a** semana
Hace aproximadamente un mes
Hace aproximadamente un año

I suggest to use the same kind of fixed strings as for hour in all cases.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use an array of intervals => their translated values to make the code shorter/easier to read

…accuracy

Removing unnecessary else

Also support 'about' times for future times

Make 'about' times translateable in all languages
@@ -820,48 +820,76 @@ public static function timeAgoInWords($dateTime, $options = array()) {
return __d('cake', 'on %s', date($format, $inSeconds));
}

$f = $accuracy['second'];
$fWord = $accuracy['second'];
Copy link
Member

Choose a reason for hiding this comment

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

Just what the framework needed. Few $fWords .

Copy link
Member

Choose a reason for hiding this comment

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

LOL

@markstory
Copy link
Member

This looks good to me 👍

jippi added a commit that referenced this pull request Aug 12, 2013
Making CakeTime display 'about X ago' when time passed is lower than acc...
@jippi jippi merged commit af1c2a1 into cakephp:master Aug 12, 2013
@ankr ankr deleted the caketime_accuracy_about branch August 12, 2013 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants