feat(web-analytics): Display visits by language #26432
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robbie-c
reviewed
Nov 26, 2024
| const parsedCountryCode = countryCode?.match(/([A-Z]{2})/)?.[0] ?? '' | ||
| return ( | ||
| <> | ||
| {countryCodeToFlag(parsedCountryCode)} |
There was a problem hiding this comment.
Let's add a fallbackFlag(language), it doesn't need many entries, but would have nl
There was a problem hiding this comment.
I've only added nl because it's probably the most impactful one that hasn't got any followups (Germany would be too, but there's de-DE and de-AT, and Poland seems to always be pl-PL)
There was a problem hiding this comment.
Once we have this running in prod I can go back and check if there's any obvious offender here
robbie-c
reviewed
Nov 26, 2024
This is doing some magic to filter by language. It's taking the browser language and then moving on to get just the language from the locale (i.e. pt from pt-BR). Because we want to be smart with the flags, however, we are collecting the country with the most views out of them. This should guarantee that people will see a British flag for en if most of their userbase is in the UK or an American flag if it's in America (or India, Nigeria, Australia, etc.) There's still a small bug where filtering won't work if you simply click on the row, that needs to be updated. See #26376
rafaeelaudibert
force-pushed
the
26376-add-filter-by-language
branch
from
6ad1fc1 to
30b9a1e
Compare
rafaeelaudibert
changed the title
|
Size Change: 0 B Total Size: 1.16 MB ℹ️ View Unchanged
|
rafaeelaudibert
commented
Nov 26, 2024
| # which is causing this to be flaky (en-GB happens sometimes), | ||
| # we'll instead assert on a reduced form where we're | ||
| # not counting the country, but only the locale | ||
| # assert results == [["en-US", 1.0, 3.0], ["pt-BR", 1.0, 2.0], ["nl-", 1.0, 1.0]] |
There was a problem hiding this comment.
robbie-c
approved these changes
Nov 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warning
We need to wait for #26409 because I've fixed some sorting issues during tests in that PRThis is doing some magic to filter by language. It's taking the browser language and then moving on to get just the language from the locale (i.e. pt from pt-BR). Because we want to be smart with the flags, however, we are collecting the country with the most views out of them. This should guarantee that people will see a British flag for en if most of their userbase is in the UK or an American flag if it's in America (or India, Nigeria, Australia, etc.)
Closes #26376
Changes
Does this work well for both Cloud and self-hosted?
Yep
How did you test this code?
Manually + unit tests on the new language code.