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

Formats numbers in the real time widget #12171

Merged
merged 2 commits into from Oct 17, 2017

Conversation

AMcNeice
Copy link

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 PRs that need a code review label Oct 12, 2017
@mattab
Copy link
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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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.

@mattab mattab merged commit da53d1a into matomo-org:3.x-dev Oct 17, 2017
@mattab
Copy link
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
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants