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

External player choice 'None' translated in Settings #2075

Merged
merged 3 commits into from
Aug 10, 2022

Conversation

9ekaitz
Copy link
Contributor

@9ekaitz 9ekaitz commented Feb 11, 2022


External player choice 'None' translatable in Settings

Important note
We may remove your pull request if you do not use this provided PR template correctly.

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

  • Bugfix
  • Feature Implementation

Related issue
closes #1670

Description
Enables the translation of the word 'None' in the external player selector in the Settings menu. In order to do that the name of the first player is changed from 'None' to a translatable string and the whole array of names is translated. (Only the 'none' one has a translation, the other ones remain the same)

Testing (for code that is not small enough to be easily understandable)
Just tested if the word is substituted by the one on the locales file

Desktop (please complete the following information):

  • OS: GNU/Linux
  • OS Version: Pop!_OS 21.04
  • FreeTube version: v0.16.0 Beta

Additional context
Is the first time I contribute to a project and also the first time using Vue, and haven't done a lot of things in JS, so if there is anything wrong with Vue or the format of the code tell me and I will try fix it

@PrestonN PrestonN enabled auto-merge (squash) February 11, 2022 19:50
@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 Feb 11, 2022
PikachuEXE
PikachuEXE previously approved these changes Feb 12, 2022
@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 Feb 12, 2022
auto-merge was automatically disabled February 13, 2022 11:03

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) February 13, 2022 11:03
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Feb 13, 2022
@PikachuEXE
Copy link
Collaborator

@PrestonN ?
The import of i18n is removed already

absidue
absidue previously approved these changes May 25, 2022
@@ -1,6 +1,6 @@
[
{
"name": "None",
"name": "Settings.External Player Settings.None",
Copy link
Member

Choose a reason for hiding this comment

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

maybe a different key should be added called "translated" that is a bool value and the translating part of the code will only use the "$t" function if translated is true to avoid warnings from this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I notice that the other names are not translated

Copy link
Collaborator

@PikachuEXE PikachuEXE May 30, 2022

Choose a reason for hiding this comment

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

Another way I can think of would be

this.$te(value) ? this.$t(value) : value

$te checks whether the translation key exists

Copy link
Member

Choose a reason for hiding this comment

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

Another way I can think of would be

this.$te(value) ? this.$t(value) : value

$te checks whether the translation key exists

that's probably the better way to do it. was not aware of the te function

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.

Let's use $te then

@9ekaitz
Copy link
Contributor Author

9ekaitz commented May 31, 2022

Let's use $te then

Should I name the null player "None" or keep the changes and name it with the translation key "Settings.External Player Settings.None"?

@PikachuEXE
Copy link
Collaborator

I think the "name": "Settings.External Player Settings.None" can be kept
And expect it to keep either the name, or the translation key
If there is no translation entry for the value, we assume that's the name (without translations)
And to do that we update
return this.$store.getters.getExternalPlayerNames to

return this.$store.getters.getExternalPlayerNames.map((value) => this.$te(value) ? this.$t(value) : value)

(Please test before committing, coz I haven't :P)

@9ekaitz
Copy link
Contributor Author

9ekaitz commented Jun 1, 2022

I think the "name": "Settings.External Player Settings.None" can be kept And expect it to keep either the name, or the translation key If there is no translation entry for the value, we assume that's the name (without translations) And to do that we update return this.$store.getters.getExternalPlayerNames to

return this.$store.getters.getExternalPlayerNames.map((value) => this.$te(value) ? this.$t(value) : value)

(Please test before committing, coz I haven't :P)

It works fine with the other players but the null one appears as Settings.External Player Settings.None in the languages in which it is not translated. Not sure if that's a problem

@9ekaitz
Copy link
Contributor Author

9ekaitz commented Jun 1, 2022

It works fine with the other players but the null one appears as Settings.External Player Settings.None in the languages in which it is not translated. Not sure if that's a problem

I think it is because the $te detects that it is not translated and instead of fallbacking to English it puts the name that is in the json (Settings.External Player Settings.None)

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Jun 2, 2022

I made some changes on my own branch
https://github.com/PikachuEXE/FreeTube/tree/feature/translated-player-name

Some screenshots:
When using zh-tw (with test translation Nein)
image

When using jp (without translation)
image

When using en (with translation)
image

Let me know what you think

Edit: The commit is PikachuEXE@410ff60 in case you want to look at diff

@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 5, 2022
auto-merge was automatically disabled June 18, 2022 08:03

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) June 18, 2022 08:03
@PikachuEXE PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Aug 1, 2022
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.

Tested locally
Took me sometime to figure which one is es

@ChunkyProgrammer
Copy link
Member

@PrestonN looks like you might need to approve this one for it to get merged

@PrestonN PrestonN merged commit f8eff0e into FreeTubeApp:development Aug 10, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 10, 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.

Please allow None to be translatable
6 participants