jackocnr / intl-tel-input

A JavaScript plugin for entering and validating international telephone numbers
https://intl-tel-input.com
MIT License
7.54k stars 1.94k forks source link

WIP adds translations #1574

Closed anthony0030 closed 4 months ago

anthony0030 commented 4 months ago

This is for #1567

jackocnr commented 4 months ago

This is so cool, thank you!

I love the way you've split up each translation file, as it allows us to pull in the country names in every language for free straight away! And then the interface translations can catch up later - what do you think?

If you're on board, I suppose this just opens up the question of which languages we should enable to start with (I see this lib includes over 600 languages 😬). I would say all of them, but then that's a lot of translation files to download and take up disk space for everyone who installs this lib in their project. Perhaps we could start with the top 10(?) most common languages, and see how that works out?

I realise this would involve tweaking the way the supported languages are configured - the easiest way would be to hard-code an array of supported language codes in the grunt/translations.js file.

To answer your questions:

Should we hard code the translation files in the src or import them like I am doing now?

I like the way you have it setup now, with interface translations in src, and then when you run the translations task, they get copied over to build, and the country names get generated from the submodule. This way we're never liable for any country names in any language - we can always defer to the submodule as the source of truth.

Do you think it is worth making a translation file that has each country in the most common language of that country?

It's an interesting idea, but I'd say let's wait and see if there's any demand for this first.

Some feedback:

Thanks again for your great work on this - I look forward to getting it merged!

anthony0030 commented 4 months ago

Answers

And then the interface translations can catch up later - what do you think?

That's a good idea.

Perhaps we could start with the top 10(?) most common languages, and see how that works out?

Uncompressed all the languages are just under 4 MB. I can get this number down to 3.2MB removing all the whitespace. The build time for all the translations is 755ms. I would like to try to get all of them in. If you are adamant about cutting the size even more. I DON'T want to be the person to select the 10 countries, this could damage my public image.

I think we should add an npm script in package.json to run the translations task.

I agree, This would make the version of grunt in the node_modules run, instead of looking for grunt on the computer.

It might be a bit confusing that in src/i18n/de.mjs we only have the interface translations - do you think we should make it clear that this is intentional somehow, e.g. nest them in src/i18n/interface/de.mjs - what do you think?

I agree, I will modify to nest in a sub-folder

I'm wondering about having the separate english translation file...

Yes, that's a good compromise. I will add code to the project to strip out the English.

Other

The code is still very rough, and I will improve it, so please don't merge until I give the go ahead please.

Please review the code changes I made, your feedback really helps.

I will see if I can get my team to translate the interface into more languages.

jackocnr commented 4 months ago

Uncompressed all the languages are just under 4 MB. I can get this number down to 3.2MB removing all the whitespace.

Thanks for looking into this, but we've had complaints about filesize before, and supporting 600 languages seems ridiculous to me.

I DON'T want to be the person to select the 10 countries, this could damage my public image

😂 I hear you, I'll take the hit. Let's start with the following:

["en", "zh", "hi", "es", "fr", "ar", "bn", "ru", "pt", "ur", "id", "de", "ja", "mr", "te", "tr", "ko", "th", "el"]

I will add code to the project to strip out the English.

Up to you - but the easier thing would just be to tweak the build files manually, and then remove "en" from the list of supported languages so it doesn't get re-generated.

please don't merge until I give the go ahead

👍

Please review the code changes I made, your feedback really helps.

Will do, although when I checked just now, Github said there were over 1k files modified, and crashed when I tried to view the diff 😂

I will see if I can get my team to translate the interface into more languages.

Amazing! Thanks so much.

anthony0030 commented 4 months ago

I understand the concern about the size I will modify the script to limit the translations to the ones you provided + anyones that I can get translated.

How do you feel about using google translate for the list you gave me and have the community fix any problems?

Please just review the changes to the JavaScript in the grunt directory.

jackocnr commented 4 months ago

How do you feel about using google translate for the list you gave me and have the community fix any problems?

Great idea 👍

Please just review the changes to the JavaScript in the grunt directory.

I've just had a play with the latest changes, and when I run the translations task, it's pulling in all 600 languages...

And also, even when it's pulled them all in, the task never seems to finish for me, it just hangs.

Finally, one thing: the compression function you wrote is cool, but thinking about it, it's probably not needed - if people are loading these modules into their code, they will likely be doing their own modification anyway, do you think?

jackocnr commented 4 months ago

Oooh I see, that's why Github says there are over 1k files changed in this PR - you've committed every single language! Can we remove (most of) those? 😂

anthony0030 commented 4 months ago

When I get back to the office tomorrow I will:

About the code not running on your computer here is a funny meme IMG_6227

Unrelated, do you understand my username?

jackocnr commented 4 months ago

LOL

Unrelated, do you understand my username?

And yes, now that you say it, I see the reference 🇬🇷 - love it - very appropriate for this plugin! ☎️

jackocnr commented 4 months ago

@anthony0030 actually I think you're right about storing the English translations in their own module, alongside the others. We should just import that module (build/i18n/en.mjs) directly into src/js/intl-tel-input.ts and use that instead of the hardcoded English strings. Maybe in intl-tel-input.ts we could change the i18n option to default to null and then in the _init method, say something like if (!this.options.i18n) { this.options.i18n = en; } - what do you think?

anthony0030 commented 4 months ago

That sounds like a good idea but instead of checking for null I would do a merge. For example, I want all the countries to be in English as they are and change just the interface (actual usecase). This would allow the user the flexibility to just overite some strings or all of them.

This is how you would achieve this.

this.options.i18n = {...en, ...this.options.i18n }

Here is some good documentation explaining this

jackocnr commented 4 months ago

🔥

anthony0030 commented 4 months ago

I cannot translate strings in bn (Bangla) in Google Translate, Skipping it for now.

jackocnr commented 4 months ago

I cannot translate strings in bn (Bangla) in Google Translate

I think Bangla is another name for Bengali, no?

anthony0030 commented 4 months ago

I think Bangla is another name for Bengali, no?

I did not know that, I am adding it then. If we are wrong someone will probably point it out 🤷🏻‍♂️

anthony0030 commented 4 months ago

I made the changes that I described yesterday. I am having problems removing the default strings. I will need some help with that, I have no experience writing typescript. There is a lot I need to do, so at the moment, I am unable to get the translations reviewed. Possibly integrate a translation management system in the future like crowdin can help.

This is what I have tried:

Screenshot 2024-04-16 at 14 25 34 Screenshot 2024-04-16 at 14 25 52

There might be a possibility that I need to make the translation build script output the country translations also in the src folder, I don't know yet tho.

Tomorrow is my son and I's birthday, So I will be spending the day with family and friends.

To make this perfect the things that still need to happen are:

jackocnr commented 4 months ago

This is amazing work, thanks so much Anthony!

The translations task is working perfectly for me now.

Tomorrow is my son and I's birthday, So I will be spending the day with family and friends.

❤️ this is lovely. Happy Birthday to both of you! Have a great day 🌻

To make this perfect the things that still need to happen are...

I'm happy to wrap this up if you like? You've already done 99% of the work, it is nothing for me to finish off. Or, if you would prefer to do it yourself that is also fine - let me know.

Re: your code above - it looks fine to me, I would just say instead of setting defaults.i18n to defaultInterfaceStrings, instead you do what you suggested, and in _init method, do something like this.options.i18n = {...defaultInterfaceStrings, ...this.options.i18n }.

And then, I would say we should import "build/i18n/en/countries" into "src/js/intl-tel-input/data" and replace the hardcoded country names in that file.

anthony0030 commented 4 months ago

Hi @jackocnr,

This is amazing work, thanks so much Anthony!

Thanks for your kind words, You are very welcome!

❤️ this is lovely. Happy Birthday to both of you! Have a great day 🌻

Thank you very much for the wishes!

I'm happy to wrap this up if you like?

Yes please, I don't have the time or the mind at the moment to learn typescript.

some clarification on the Improve translation Importing in ES it would be great to be able to import translations like this

import el from "intl-tel-input/i18n";
// and
import {el, it} from "intl-tel-input/i18n";

NOT TO NEED

import el from "intl-tel-input/build/i18n/el/index.mjs";

I could not get it to do this.

Re: your code above...

That sounds good, I could not find the correct place to put it. and I could not get it to work. src/js/intl-tel-input.ts(2,37): error TS2307: Cannot find module '../i18n/interface/en' or its corresponding type declarations.

PS: I just pushed up some code for listing variable names.

jackocnr commented 4 months ago

Manually merged and released in v21.2.0. Thanks again Anthony!

btw I got it working so you can do import fr from "intl-tel-input/i18n/fr" which seems pretty good 👍

anthony0030 commented 4 months ago

Cool I will take a look on Thursday! You are very welcome! I look forward to working with you again in the future!

jackocnr commented 4 months ago

You can now view a live demo of all locales on storybook: https://intl-tel-input.com/storybook/?path=/docs/intltelinput--i18n