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

Improve number formats #8857

Merged
merged 20 commits into from Oct 11, 2015

Conversation

Projects
None yet
3 participants
@sgiehl
Member

sgiehl commented Sep 26, 2015

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

@sgiehl sgiehl added this to the 2.15.0 milestone Sep 26, 2015

"OneDay": "1 hari",
"OneMinute": "1 menit",
"OneMinuteShort": "1 mnt",
"OriginalLanguageName": "Bahasa Indonesia",
"OriginalLanguageName": "Indonesia",

This comment has been minimized.

@mattab

mattab Oct 2, 2015

Member

this is regression in language name, see https://id.wikipedia.org/wiki/Bahasa_Indonesia

@mattab

mattab Oct 2, 2015

Member

this is regression in language name, see https://id.wikipedia.org/wiki/Bahasa_Indonesia

This comment has been minimized.

@sgiehl

sgiehl Oct 2, 2015

Member

That is what CLDR returns. They have done an update within the last month. So some strings might have changed

@sgiehl

sgiehl Oct 2, 2015

Member

That is what CLDR returns. They have done an update within the last month. So some strings might have changed

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 2, 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

  • add number formatting to scheduled reports (email at least, SMS not needed)
  • add number formatting to sparklines reports (Visitors, Referrers and Goals/Ecommerce overviews)
  • Indicate in Dev changelog any change to API output - refs #8856 prettyDate value has changed
Member

mattab commented Oct 2, 2015

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 &#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

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

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Oct 2, 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...

Member

sgiehl commented Oct 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 4, 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 :)

Member

mattab commented Oct 4, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 9, 2015

Member

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

Member

mattab commented Oct 9, 2015

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

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Oct 11, 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

Member

sgiehl commented Oct 11, 2015

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

@sgiehl

This comment has been minimized.

Show comment
Hide comment
@sgiehl

sgiehl Oct 11, 2015

Member

Guess it would be better to use $this->symbolPlus here, as the +sign might be language dependent

Member

sgiehl commented on core/NumberFormatter.php in 95bea8a Oct 11, 2015

Guess it would be better to use $this->symbolPlus here, as the +sign might be language dependent

mattab pushed a commit that referenced this pull request Oct 11, 2015

@mattab mattab merged commit 7d2c0fa into master Oct 11, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@mattab mattab deleted the numberformats branch Oct 11, 2015

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 11, 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

Member

mattab commented Oct 11, 2015

Hi @sgiehl nice progress!

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

@ThaDafinser

This comment has been minimized.

Show comment
Hide comment
@ThaDafinser

ThaDafinser Oct 12, 2015

Contributor

When php-cs-fixer finally get in, we can use php-cs-fixer fix core --fixers=long_array_syntax

Contributor

ThaDafinser commented on core/NumberFormatter.php in 7558648 Oct 12, 2015

When php-cs-fixer finally get in, we can use php-cs-fixer fix core --fixers=long_array_syntax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment