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

Jump to bottom

Improve number formats #8857

Merged
merged 20 commits into from Oct 11, 2015
Merged

Improve number formats #8857

merged 20 commits into from Oct 11, 2015

Conversation

sgiehl
Copy link
Member

@sgiehl 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 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. labels Sep 26, 2015
@sgiehl sgiehl added this to the 2.15.0 milestone Sep 26, 2015
@sgiehl sgiehl force-pushed the numberformats branch 2 times, most recently from 78de477 to 3ececd9 Compare September 26, 2015 15:22
mattab
mattab reviewed Oct 2, 2015
View reviewed changes
plugins/Intl/lang/id.json
"OneDay": "1 hari",
"OneMinute": "1 menit",
"OneMinuteShort": "1 mnt",
"OriginalLanguageName": "Bahasa Indonesia",
"OriginalLanguageName": "Indonesia",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@mattab
Copy link
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   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 Improve date & time formats #8856 prettyDate value has changed

@mattab mattab mentioned this pull request Oct 2, 2015
Closed
@sgiehl
Copy link
Member Author

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
Copy link
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
Copy link
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 :-)

@gaumondp gaumondp mentioned this pull request Oct 9, 2015
Closed
@sgiehl sgiehl force-pushed the numberformats branch 5 times, most recently from c4a5d01 to df0c999 Compare October 10, 2015 23:06
@sgiehl sgiehl force-pushed the numberformats branch 4 times, most recently from 0c87f5f to f7b6ef1 Compare October 11, 2015 12:06
@sgiehl
Copy link
Member Author

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

mattab pushed a commit that referenced this pull request Oct 11, 2015
Merge pull request #8857 from piwik/numberformats
7d2c0fa 
Improve number formats
@mattab mattab merged commit 7d2c0fa into master Oct 11, 2015
@mattab mattab deleted the numberformats branch October 11, 2015 22:56
@mattab
Copy link
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

This was referenced Oct 11, 2015
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sgiehl sgiehl

Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sgiehl @mattab @ThaDafinser