Note that this will break for any translations that don't include a $1. I'd suggest going through each translation to either replace Ethereum with $1, or if that seems difficult then deleting this string so that we can ask for a new translation.

I've talked with @danjm and he mentioned I should only care about en translations.

I've actually just tested what you mentioned and it should work just fine for other languages without $1. I just removed the $1 from the English translation and it shows whatever text is there right now (see below on the screenshot). That means that other languages will show Ethereum until their strings contain $1, which should be done by our localization team. If you can let me know what's the process of engaging our localization team to get it updated for other languages, that would be great!

That will flood sentry with errors. Our localization system expects each message to have the same number of substitutions across all locales

Good to know. I've tried to replace all Ethereum occurrences with $1, but I don't see Ethereum in the ko/messages.json file:

So, either it's not there or it's there, but I don't know where. If you give me a contact to someone from our localization team, I can check with them.

@dan437 Sorry for misleading you. When adding new translations, we only need to worry about en translations.When updating existing translations, if there is an obvious change to make to non-english translations, we can do that manually. If not, it is usually best to add a translation as new.

According to google translate, 이더 리움 is korean for ethereum

which appears to be the fifth set of characters from the left

This does hint that localizing the actual network names is something we should do as well.

That's interesting, thanks, Dan and Mark! I've replaced 이더리움 with $1 and will make sure to localize network names as well.

These strings should be localized. "Test" probably isn't a very good name in all locales for example.

I believe that network names should be the same across all languages. I like to think of it as names of companies.. we also don't translate them. 'Test' is a special one just for testing purposes and I'm open to changing it if there is something better. I thought that devs are used to hear Test Net(work), so that's why I chose "Test".

I've changed it to "Testnet", because I believe that some translations don't use the word "network" after our dynamic value, so it would just say something like: "The network fee covers the cost of processing your swap and storing it on the Test".

From the official Jest site: "The snapshot artifact should be committed alongside code changes, and reviewed as part of your code review process."

I will be adding many more unit tests for the Swaps feature in upcoming PRs.

The translation system already defaults to using the en translation - this fallback is redundant.

Awesome, will remove it.

Done.

It might be worth adding 이더리움 as a translation for networkNameEthereum so we don't have a regression here.

Done.

Nit: In general we should avoid passing variables to t for the first parameter, because our localized string linting script then has a more difficult time discovering whether the string is used or not. There is a 'strict mode' that we've been slowly introducing one file at a time that assumes t is passed a string literal, so any use of a variable will make that task more difficult.

The alternative I'd suggest is deleting SWAPS_CHAIN_ID_TO_NETWORK_NAME_KEY_MAP, and replacing it with a function that accepts t and the chain ID, and returns the correct network name.

Edit: Updated to be a bit more clear

In general we should avoid passing variables to t, because our localized string linting script then has a more difficult time discovering whether the string is used or not

This is a good point, I always forget this.

The alternative I'd suggest is deleting SWAPS_CHAIN_ID_TO_NETWORK_NAME_KEY_MAP, and replacing it with a function that accepts t and the chain ID, and returns the correct network name.

Wouldn't that still result in a variable passed to t?

Wouldn't that still result in a variable passed to t?

Ah, no, I see. The function would map chainId to calls to t using string literals

So, there is https://github.com/i18next/react-i18next , which is one of the best internationalization frameworks in the world for React / React Native. Interpolation (passing dynamic values into translations) is one of the most used functionalities there: https://www.i18next.com/translation-function/interpolation

I believe we should support passing dynamic values into translations, since it's a very common thing to do. With i18next it's done like this:

{
    "key": "{{what}} is {{how}}"
}
i18next.t('key', { what: 'i18next', how: 'great' });

We're not talking about interpolation here. Our translation helper does support interpolation.

We've just clarified it over Slack - the issue is not about passing variables in the second param to t, but in the first one. Will create a function as suggested.

Maybe we should throw here instead of defaulting to 'networkNameEthereum'? 🤔 This should be unreachable right now, and in the future could only be hit if we add support for a fourth network in swaps but forget to add a network name. That scenario would be worth throwing an error for.

Unless there's some case I'm missing.

It sounds fine to me. What about something like this?

if (!networkNameKey) {
  throw new Error('This network is not supported for token swaps');
}

That works!