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

Improve system locale selection #2271

Merged

Conversation

ChunkyProgrammer
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer commented May 27, 2022


Improve system locale selection

Pull Request Type
Please select what type of pull request this is:

  • Bugfix
  • Feature Implementation

Related issue
None

Description
For locale set to system, If a user's system locale is not found then it will default to a locale with the same language instead of defaulting to 'en-US'

Testing (for code that is not small enough to be easily understandable)
Has this pull request been tested?
Yes this PR has been tested by hardcoding the value here to a different locale:
https://github.com/FreeTubeApp/FreeTube/pull/2271/files#diff-ef55248451a73d0a2680edeb9d5bbd8a6d0a3deaa89406c100d29fef7a45e497R236

Desktop (please complete the following information):

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.16.0

@ChunkyProgrammer ChunkyProgrammer changed the title Improve system language selection Improve system locale selection May 27, 2022
@PrestonN PrestonN enabled auto-merge (squash) May 27, 2022 14:10
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 27, 2022
@efb4f5ff-1298-471a-8973-3d47447115dc

While testing this my locale wasnt found and it still defaulted to language based on IP

@ChunkyProgrammer
Copy link
Member Author

ChunkyProgrammer commented May 27, 2022

While testing this my locale wasnt found and it still defaulted to language based on IP

This PR only affects FreeTube UI language. (It will see if there's a weblate language that matches closely to it).

For example, if the system locale is set to "es-US" for spanish US then setting FreeTube's language to "system" will make the UI match the "es" translation instead of the default "en-US"

absidue
absidue previously approved these changes May 30, 2022
targetLocale = Object.keys(i18n.messages).find((locale) => {
const localeName = locale.replace('-', '_')
return localeName.includes(systemLocale.replace('-', '_'))
const systemLocaleName = systemLocale.replace('-', '_')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe good to put example value(s) in code comments (whole section not just this line)
I can't understand this code section easily

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

@efb4f5ff-1298-471a-8973-3d47447115dc
I guess I should mark as requested for change - -?

I think the variable names are quite ambiguous
i.e.

  • systemLocale
  • systemLocaleName
  • systemLocaleLang

One of the things I can think of is to add example value(s) in form of code comments
So that the readers know what kind of values those variables would store

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jun 1, 2022
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

LGTM

@PrestonN PrestonN merged commit 5f8e33c into FreeTubeApp:development Jun 3, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants