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

Formats numbers in the real time widget #12171

Merged
merged 2 commits into from Oct 17, 2017

Conversation

Projects
None yet
3 participants
@AMcNeice
Contributor

AMcNeice commented Oct 12, 2017

Brings the formatting of the real time widget inline with the rest of the number formatting.

Addresses issue: #11967

@mattab mattab added this to the 3.2.1 milestone Oct 12, 2017

@mattab mattab added the Needs Review label Oct 12, 2017

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 12, 2017

Member

Thanks for the pull request @AMcNeice and welcome to our project 👍

we'll review this in the coming days/weeks!

Member

mattab commented Oct 12, 2017

Thanks for the pull request @AMcNeice and welcome to our project 👍

we'll review this in the coming days/weeks!

<td class="column">{{ visitorsCountToday }}</td>
<td class="column">{{ pisToday }}</td>
<td class="column">{{ visitorsCountToday|number }}</td>
<td class="column">{{ pisToday|number }}</td>

This comment has been minimized.

@sgiehl

sgiehl Oct 12, 2017

Member

Haven't tested your changes yet, but isn't the formatting done twice this way.
The number filter for twig uses this method, which calls NumberFormatter::format, but that is already done in Controller. So guess it should be enough to do it either in Controller or in View.

@sgiehl

sgiehl Oct 12, 2017

Member

Haven't tested your changes yet, but isn't the formatting done twice this way.
The number filter for twig uses this method, which calls NumberFormatter::format, but that is already done in Controller. So guess it should be enough to do it either in Controller or in View.

This comment has been minimized.

@AMcNeice

AMcNeice Oct 16, 2017

Contributor

@sgiehl - Makes sense, missed the twig formatting referencing that function. I'll test locally and push up some changes that only has the formatting in the view so that all of the items are calling the same helper function.

@AMcNeice

AMcNeice Oct 16, 2017

Contributor

@sgiehl - Makes sense, missed the twig formatting referencing that function. I'll test locally and push up some changes that only has the formatting in the view so that all of the items are calling the same helper function.

@sgiehl

sgiehl approved these changes Oct 17, 2017

@mattab mattab merged commit da53d1a into matomo-org:3.x-dev Oct 17, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 17, 2017

Member

Thanks for the PR @AMcNeice 👍

Member

mattab commented Oct 17, 2017

Thanks for the PR @AMcNeice 👍

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