Made Last Seen more human readable #9112
Conversation
| $seconds = floor($reminder - $minutes * 60); | ||
| $time = sprintf("%02s", $hours) . ':' . sprintf("%02s", $minutes) . ':' . sprintf("%02s", $seconds); | ||
| $time = sprintf("%02s", $days) . ':' . sprintf("%02s", $hours) . ':' . sprintf("%02s", $minutes) . ':' . sprintf("%02s", $seconds); | ||
| $centiSeconds = ($numberOfSeconds * 100) % 100; | ||
| if ($centiSeconds) { |
There was a problem hiding this comment.
Should we maybe display days only if there was actually a day? Also it formats a time here, I'm not sure if eg 01:01:01:01 would be clear that the first one is actually meant as days? Not sure how one would format something like this. Maybe we'd need to generate full date in this case? eg something like 2015-01-01 01:01:01?
There was a problem hiding this comment.
Please have a look at the latest commit.
I've tried to make it something like "5 days, 05:03:01", and the days part is only visible when days > 0.
|
@tsteur Please have a look at the updated version! |
| if($days == 0) | ||
| $time = sprintf("%02s", $hours) . ':' . sprintf("%02s", $minutes) . ':' . sprintf("%02s", $seconds); | ||
| else | ||
| $time = sprintf("%02s days, ", $days) . sprintf("%02s", $hours) . ':' . sprintf("%02s", $minutes) . ':' . sprintf("%02s", $seconds); |
There was a problem hiding this comment.
"days" needs to be translated. You can see it a few lines further below how it works. You want to use the translation key Intl_NDays. Eg like Piwik::translate('Intl_NDays', $days)
There was a problem hiding this comment.
In the test I just noticed it says 01 days, 10:00:00 I think 1 days instead of 01 days would be better here. Ideally it would say 1 day and 3 days but we don't have to take care of this now as we always use plural so far everywhere
|
Are you familiar with unit tests? It would be good to have this code covered by tests. Existing tests are located in |
|
@tsteur I've made the changes as per your comments, please have a look! I'm not really familiar with the tests (I'm new) but I'll have a look and try to understand. |
|
@tsteur As per my understanding, the tests are failing because they are still expecting the older values. |
|
@tsteur I've updated the expected values in the test file. :) |
|
@tsteur There are still some fails caused due to my edit in the test file. I'm not sure how to go about this. |
| if ($days == 0) { | ||
| $time = sprintf("%02s", $hours) . ':' . sprintf("%02s", $minutes) . ':' . sprintf("%02s", $seconds); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Just for the next time: In our coding style we usually use } else { in one line but it's alright for now :)
|
The failing tests are not related to your change. All good :) |
|
@tsteur Thank you so much for your continuous guidance! :) |
|
Hi @ritvikgautam thanks for PR! maybe you could also squash your commits into one bigger commit? http://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit |
|
@ritvikgautam If you're familiar with squashing commits it would be good to give it a try (see blog post) otherwise let us know |
|
I was just waiting for an answer re the squashing of commits ( http://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit ) then I will merge. Are you familiar with squashing commits or can you give it a try maybe? What happens there is that you squash those 5 commits into one commit via in your case something like |
|
@tsteur It's all a little bit confusing for me. Will it be okay if I just create a new pull request? |
|
Sure that's fine as well @ritvikgautam |
Update Formatter.php Update Formatter.php Made changes in the coding style, added translation for 'days'. Update FormatterTest.php Made some changes in the expected array. Update Formatter.php Code style. Made the last seen time more human readable by incorporating days in the time format.
|
Awesome, thx for that :) |
Made Last Seen more human readable
mattab
added
the
Regression
I've updated the last seen format as per #8956