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.

Sign up for GitHub

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

Jump to bottom

Improve date & time formats #8282

Closed
wants to merge 14 commits into from
Closed

Improve date & time formats #8282

wants to merge 14 commits into from

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jul 6, 2015

This PR aims to improve the date & time formats used by Piwik.

❗ This PR will also change most of the visible english date/time formats

Current behavior

  • Currently there are a few date formats defined in translation strings like CoreHome_DateFormat, CoreCome_MonthLong, CoreHome_ShortWeekFormat and others. Those keys contain placeholders like %shortMonth%, %longYear%, %longDay and others, beeing replaced while getting a localized date. See Date.php
    Those formats have already been localized to some language. Unfortunately sometimes someone made errors in translating those format strings, which resulted in incomplete/incorrect dates in piwik for that language.
  • Additionally to those localize-able formats there are quite a few occurrences in the code where the format is directly defined, and thus not language depended.
  • Names of Months and Days were already translatable before they had been moved to the Intl-Plugin.
  • Range formats such as the format for a week (CoreHome_ShortWeekFormat) had kind of fixed format patterns, making no difference depending on the range beeing within a month, year or not.
  • Usage of time format was fixed to H:i:s resulting in not localized time formats

Behaviour with this PR

  • All date and time formats are now fetched from CLDR, using all additional symbols for all languages
  • Fixed format occurences in the code have been replaced to use localized formats
  • Names of Days and Months are still fetched from CLDR, but in both variants. Additionally there is now a "Stand-Alone" variant, which will be used in some formats for some languages or when those names are used as "Stand-Alone"
  • Range formats are now fetched from CLDR. They now respect the time difference, which means the format differs if the minimal difference is day, month or year
    Examples for english range formats:
    • Range within one month: Oct 7 – 13, 2024
    • Range within one year Nov 25 – Dec 1, 2024
    • Range within more than one year Dec 30, 2024 – Jan 5, 2025
  • Time formats are now localized, which also means languages like english are now using the 12 hour format (am/pm)

@sgiehl sgiehl added Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. RFC Indicates the issue is a request for comments where the author is looking for feedback. labels Jul 6, 2015
@mattab mattab added this to the 2.14.1 milestone Jul 7, 2015
@sgiehl sgiehl force-pushed the date_formats branch 3 times, most recently from 389b123 to 844e3f7 Compare July 8, 2015 21:21
@mattab mattab modified the milestones: 2.14.1, 2.15.0 Jul 10, 2015
@sgiehl sgiehl force-pushed the date_formats branch 2 times, most recently from be5f09d to 378a726 Compare July 13, 2015 18:50
@mattab mattab mentioned this pull request Jul 15, 2015
Closed
@sgiehl sgiehl removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jul 15, 2015
@mnapoli
Copy link
Contributor

mnapoli commented Jul 20, 2015

@sgiehl Date formats have changed on master (e.g. http://builds-artifacts.piwik.org/ui-tests.master/14276.7/ActionsDataTable_segmented_visitor_log or http://builds-artifacts.piwik.org/ui-tests.master/14276.7/EvolutionGraph_export_image) is it by any chance related to this (or another PR maybe)? (sorry if I'm a little out of topic :) ) In any case, are the new formats good? (i.e. should I update the screenshots?)

@sgiehl
Copy link
Member Author

sgiehl commented Jul 20, 2015

@mnapoli That shouldn't be on master, as this PR is not merged. Those screenshots were build on the branch: See https://travis-ci.org/piwik/piwik/builds/71750226

The changes done here are still for discussion. Maybe some formats should be adjusted, but I tried to use those fitting best.

@mnapoli
Copy link
Contributor

mnapoli commented Jul 20, 2015

Ah thank you, I was tricked by the "PR build" of travis which ends up in the "master" branch in our build-artifacts servers. All good then!

@sgiehl sgiehl force-pushed the date_formats branch 4 times, most recently from 308c482 to f099bdd Compare July 24, 2015 21:07
@sgiehl sgiehl added the Needs Review PRs that need a code review label Jul 24, 2015
tsteur
tsteur reviewed Jul 27, 2015
View reviewed changes
core/Date.php
*/
public function getLocalized($template)
{
$template = $this->replaceLegacyPlaceholders($template);
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to do this only if $template starts with %? I'm not sure how often getLocalized is called... If it is not often called it won't be needed I reckon. Thinking more re performance

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only called when outputting some dates. But I guess we could check for % and only do the legacy stuff then

@tsteur
Copy link
Member

tsteur commented Jul 27, 2015

Just BTW: At some point (for Piwik 3.0 or 4.0) it would be nice to split the Date classes in different classes. Eg Date and DateFormatter or maybe more classes. Using an existing lib would be even better but probably very hard to replace the current one without regressions.

@quba
Copy link
Contributor

quba commented Jul 27, 2015

We might use something like http://carbon.nesbot.com/

@sgiehl
Copy link
Member Author

sgiehl commented Jul 27, 2015

@quba that one does not contain any date formats, also the translations are very "minimalistic"
@tsteur I had a look at available implementations, but most of them do not contain all we need, and if they contain much more and a very big

@quba
Copy link
Contributor

quba commented Jul 27, 2015

Understood. So maybe we should create our own library? i know that it's not that easy but would be much better.

@tsteur
Copy link
Member

tsteur commented Jul 27, 2015

Yeah I thought it will be hard to find one that has all the features we have or to find maybe one that has most features and that we can extend. If we find one it will be hard to replace the lib and to make sure everything still works as our date lib has some "special behaviours" :) I reckon splitting the Date into different classes might be a good start. I'm not sure how well tested it is but shouldn't be too hard to write tests for it so this might be the easiest for now (unless there's a really useful library)

@sgiehl
Copy link
Member Author

sgiehl commented Jul 29, 2015

@tsteur I've done the commented changes. Btw. I don't think we should merge that for 2.15.0
I guess it would be better for 3.0 as the changes in time/date formats might break some plugins.
Should I rebase on the 3.0 branch and issue a new PR for that?

@tsteur
Copy link
Member

tsteur commented Jul 29, 2015

Merging directly into 3.0 sounds good to me if not needed earlier. Not sure if you have to rebase. Might be possible to just create another PR and select "3.0" there. LGTM

@sgiehl
Copy link
Member Author

sgiehl commented Jul 29, 2015

Will be rebased and merge to 3.0 branch

@sgiehl sgiehl closed this Jul 29, 2015
@sgiehl sgiehl mentioned this pull request Jul 29, 2015
Merged
@mattab mattab added the duplicate For issues that already existed in our issue tracker and were reported previously. label Aug 10, 2015
@sgiehl sgiehl mentioned this pull request Sep 26, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. duplicate For issues that already existed in our issue tracker and were reported previously. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Needs Review PRs that need a code review RFC Indicates the issue is a request for comments where the author is looking for feedback.
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sgiehl @mnapoli @tsteur @quba @mattab