@sgiehl opened this Pull Request on September 26th 2015 Member

This PR aims to improve the number formats used by Piwik. (refs #1254)

Currently the number formats mostly depend on the server locale. So on some systems the decimal separator is a . where on another system it might be a ,

With this changes number formatting data will be included in the Intl Plugin. With a new NumberFormatter in Core, as well as a plugin for jQPlot the number formatting will be done depending on the language.

The new number formatting makes a difference between numbers only and percentage values. As in some languages it is 35% where in others it might be % 35. (Note: we could later use that for money formats aswell)

Places where new number formatting is currently used:

  • Datatables (automatically done for all numbers)
  • all jqplot graphs (axis values and tooltips)
  • MetricFormatter (might affect some API results)

This branch is based on dateformats branch. So it needs a rebase after #8856 is merged

@mattab commented on October 2nd 2015 Member

Wow, we have number formatting, finally! Awesome added value @sgiehl

I love it already. the UI just looks better with a simple change like this. reports become easier to read and analyse!

Feedback

  • There are three system tests failing (added comma as thousands separators)
  • One of them introduces a No Breaking space character where maybe we should avoid it? (U+00A0 \xc2\xa0   NO-BREAK SPACE) here:
-    <bounce_rate>100%</bounce_rate>
-    <exit_rate>100%</exit_rate>
+    <bounce_rate>100&#xA0;%</bounce_rate>
+    <exit_rate>100&#xA0;%</exit_rate>
  • Could you add a few tests for NumberFormatter?

suggestions

  • [x] add number formatting to scheduled reports (email at least, SMS not needed)
  • [x] add number formatting to sparklines reports (Visitors, Referrers and Goals/Ecommerce overviews)
  • [x] Indicate in Dev changelog any change to API output - refs #8856 prettyDate value has changed
@sgiehl commented on October 2nd 2015 Member

Rebased on master.

@mattab Regarding your feedback:
I've changed the MetricFormatter to use the new formatting instead of using number_format, but that changed the API responses. Imho we should have API responses in english format always (or completely unformatted). Otherwise it might get harder to work with them afterwards. You've already mentioned the example with the non breaking space in french format.

Maybe I should revert the changes to MetricFomatter and rewrite it to use no or english formats always?

I'll have a look at the other suggestions...

@mattab commented on October 4th 2015 Member

Maybe I should revert the changes to MetricFomatter and rewrite it to use no or english formats always?

To make sure we don't regress API output in LTS, here is a suggestion:

  • In LTS 2.x we don't modify the API response
  • In 3.0.0 branch we can modify API response

What do you think?

I'll have a look at the other suggestions...

Thanks, looking forward to merge :)

@mattab commented on October 9th 2015 Member

Hi @sgiehl it would be awesome to get number formatting before next week so we can cut RC next week :-)

@sgiehl commented on October 11th 2015 Member

Small update: I've added support for currency formats and added usage of number formats to scheduled reports, the numbers besides the sparklines, seo metrics, visitor map, row evolution and transition popover.
Also I've undone the changes that had an impact on the api results

@mattab commented on October 11th 2015 Member

Hi @sgiehl nice progress!

Merging the PR, will make the build green, then cut a beta and test further on demo.piwik.org

This Pull Request was closed on October 11th 2015
Powered by GitHub Issue Mirror