google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Graph showing wrong currency on Y axis. #6807

Closed mohitwp closed 10 months ago

mohitwp commented 1 year ago

Bug Description

While using oi.ie site. Under AdSense (Performance for the last 28 days) widget showing earnings in euro 💶 but on Y axis currency listed as $. Ideally, both currency should match and graph currency must get update as per data.

Steps to reproduce

  1. Set up site kit using oi.ie.
  2. Set up AdSense.
  3. Click on Monetization chip.
  4. See listed currency under "performance for the last 28 days" graph Y axis.

Screenshots

image


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

This PoC can serve as a starting point for the IB.

Test Coverage

QA Brief

Changelog entry

mxbclang commented 1 year ago

@aaemnnosttv This feels to me like something we should fix sooner than later as it just doesn't look very nice. Let me know what you think!

aaemnnosttv commented 1 year ago

This is a limitation of Google charts which defers this kind of formatting to the configured locale of the charts library as a whole. This isn't ideal because the currency of the data isn't specific to a geographic locale.

image

https://developers.google.com/chart/interactive/docs/gallery/linechart#data-format

We currently load the charts with the default locale, but even if we passed through the user's locale (used for formatting numbers, etc) this wouldn't exactly solve the problem because it would still be formatting the currency based on the locale, not the data it's plotting which we don't know about until it comes from the API of course.

Not a great experience, but I'm not sure what we can really do about this for now 😕

techanvil commented 11 months ago

I've looked into this and it turns out we can address this by using the vAxis.format decimal pattern syntax, in conjunction with loading the Google Chart in the user's locale (this is necessary in order to fully support locale-specific number formatting).

I've put together a PoC - here's how it looks with the WP language set to French (note this is a development site so SK's own text isn't translated, but it would be in a real-world scenario):

image

It seems to me that loading the charts in the user's locale is the right thing to anyway, so I've added AC to this issue which include that aspect.

tofumatt commented 11 months ago

@techanvil Does setting the locale to French force the Euro currency in this case? Locales and currencies shouldn't be tied together like that 😓

I suppose if Google Charts won't let us manually specify currency there's not much we can do, but French locale could warrant all sorts of non-Euro currencies, just as a for instance. 🤔

techanvil commented 11 months ago

Hey @tofumatt, the locale and currency are decoupled by virtue of specifying our own pattern for vAxis.format, rather than specifying the built-in currency format. In the previous example the vAxis.format value is determined to be #,###,##0.00 € - which is the French format barring the group , and decimal . separators, which the Google Chart always expects as the comma and period respectively. Setting the locale on the chart then ensures it renders the locale-specific separators (e.g. these are swapped in the French notation). Note that the currency symbol in the format pattern is determined from the currencyCode provided by the AdSense report.

To illustrate further here's the same chart with Japanese selected in WP:

image

And here it is in en-US:

image

With the vAxis.format value being determined as €#,###,##0.00 in both of these cases.

tofumatt commented 11 months ago

Awesome, okay, thanks for the explanation 😅

In that case: all is good! 🙂

tofumatt commented 11 months ago

@derweili Looks good, but can you add an estimate for the issue? I'm guessing around an 11 but want to confirm with you 🙂

tofumatt commented 11 months ago

IB ✅

wpdarren commented 10 months ago

QA Update: ✅

Verified:

Screenshots ![image](https://github.com/google/site-kit-wp/assets/73545194/17ae8a6a-5161-4de8-9fc8-0670da5d8b6b) ![image](https://github.com/google/site-kit-wp/assets/73545194/9b2b9801-2bf4-4971-9766-d8d4ca0bd657) ![image](https://github.com/google/site-kit-wp/assets/73545194/c8053806-c3ca-447e-adb7-d966ed209372) ![image](https://github.com/google/site-kit-wp/assets/73545194/42e1d388-1824-4c2e-903c-799808e616ab) ![image](https://github.com/google/site-kit-wp/assets/73545194/6fedfd6c-1e3a-42b6-9640-670dc9b65585)