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

fix(rtl): Sync list of RTL languages with server backend #801

Closed
wants to merge 1 commit into from

Conversation

nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen added bug Something isn't working 3. to review labels Sep 23, 2024
@nickvergessen nickvergessen self-assigned this Sep 23, 2024
'ps', // Pashto,
'ug', // 'Uyghurche / Uyghur
'ur_PK', // Urdu
'uz', // Uzbek Afghan
Copy link
Contributor

Choose a reason for hiding this comment

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

Thats wrong, 'uz' is Uzbek which is LTR using latin script.
What you mean is uz_AF which is the afghan dialect writtin RTL.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

I am not sure here, I would like to have this as defensive as possible.
This are all valid right-to-left languages, so what if we add one in the future? All apps would need to update the library or otherwise they would be broken.

I do not see any disadvantage by keeping all valid language codes.

'he', // Hebrew
'ps', // Pashto,
'ug', // 'Uyghurche / Uyghur
'ur_PK', // Urdu
Copy link
Contributor

Choose a reason for hiding this comment

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

This will never be the case as the lang tag on the HTML element which we use to set the language only allows BCP47.
Meaning it must be ur-PK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickvergessen
Copy link
Contributor Author

I would like to have this as defensive as possible.

I tried to be as defensive as possible as well, just from another angle. I removed all languages which are not an option on transifex. We have to list RTL languages specifically anyway, as others are not allowed to contain RTL characters. If a language is growing to a "valid" percentage (server requires 50% translated before syncing them) we will notice it anyway and can start improving/listing it.

Looking at the translated percentage, we really only have ar, fa and he to care about at the moment anyway. All others that are marked translated ship 80+% english strings and when clicking around in the UI there are mostly only apps names in translated language

@ShGKme
Copy link
Contributor

ShGKme commented Sep 27, 2024

I tried to be as defensive as possible as well, just from another angle. I removed all languages which are not an option on transifex. We have to list RTL languages specifically anyway, as others are not allowed to contain RTL characters. If a language is growing to a "valid" percentage (server requires 50% translated before syncing them) we will notice it anyway and can start improving/listing it.

Looking at the translated percentage, we really only have ar, fa and he to care about at the moment anyway. All others that are marked translated ship 80+% english strings and when clicking around in the UI there are mostly only apps names in translated language

What issue having only the list of these languages in the library instead of all rtl languages does fix?

@susnux
Copy link
Contributor

susnux commented Sep 27, 2024

I removed all languages which are not an option on transifex.

Transifex supports those languages they are just not translated yet:
https://explore.transifex.com/languages/

But yes this current list is also invalid as some entries like arz are not valid bcp47 and never will work (correct is ar-EG).

@nickvergessen
Copy link
Contributor Author

Replaced by #803

@nickvergessen nickvergessen deleted the bugfix/noid/fix-rtl-direction-languages branch September 27, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants