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

Add support for locales (aka culture) #170

Closed
wants to merge 13 commits into from

Conversation

cristiingineru
Copy link

Original PR #167

There are multiple types of numerals, even the arabic numberals have multiple representations: https://en.wikipedia.org/wiki/Eastern_Arabic_numerals#Numerals

This change is adding support for different type of "cultures" by taking advantage of the standard Intl.NumberFormat

The language itself is not enough to format the numerals because there are languages which support multiple formats.

Questions:

  1. Should both the arguments of the Intl.NumberFormat constructor - locales and options - be further exposed through the js-lingui API?
  2. Should the locales be further exposed as locales or culture? Other languages are using culture, or cultureNames...
  3. Should the information regarding the culture (or locales) be passed as arguments as in this PR or stored in i18n?

@tricoder42
Copy link
Contributor

  1. jsLingui already exposes options in number and date formats. It just needs to rename format parameter to options to keep it consistent with Intl.

  2. I would stick to Intl naming here: locale

  3. I would like to support both - setting a default (probably numberLocale, since it can be different than default locale), but also allow overriding in plural and selectOrdinal

What do you think? I'm open to suggestions here

@cristiingineru
Copy link
Author

  1. Didn't notice the number and date formats :-) I've rename the arguments of these to formatters to be consistent with Intl (hopefully this will not propagate to the public API of js-lingui) and also changed the _plural function to reuse the number formatter.

  2. locales is definitely better than culture, so it was renamed accordingly to match the Intl naming

  3. I've did some monkey work to add a default locales to the i18n object. I tried to keep the object immutable so there is no way for the new member to be changed other than recreating a new i18n object using the setupI18n factory. Do you prefer i18n.numberLocales instead?

Of course, having a default complicated the things a little bit, but hopefully not that much for the user. The priority of using locales is: i18n.language < i18n.locales < prop of the Select component: https://github.com/SectorLabs/js-lingui/blob/b65a140b070280fa4d07134c472f29ab578cc71d/packages/core/src/select.js#L28

Taking into account i18n.language as a first default for locales of Intl should help not breaking any existing user code.

The options can now be passed to Intl from a Select component and there is no default in the i18n object because Intl has already a default which should work in most of the cases... I guess...

There was a parseInt(value) which is now replaced with Number(value) because apparently there is at least a use case for non integer values: percentage https://github.com/SectorLabs/js-lingui/blob/b65a140b070280fa4d07134c472f29ab578cc71d/packages/react/src/Select.test.js#L136

But, not sure about the offset and the exact use cases. Should the offset be considered as int or Number?

const translation =
pluralForms[value.toString()] || // exact match
pluralForms[i18n.pluralForm(diff, type)] || // plural form
other // fallback
return translation.replace("#", diff.toString())
return translation.replace("#", diffAsString)
Copy link
Contributor

Choose a reason for hiding this comment

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

@tricoder42 Shouldn't the value at:

return norm.map(m => (isString(m) ? m.replace("#", value) : m))
be also passed through the number() function?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it should be implicit. I see your point, thought.

One possible workaround might be using number formatter directly:
{value, plural, one {# volt} other {# volts}

vs

{value, plural, one {{value, number, ...} volt} other {{value, number, ...} volts}}

but that wouldn't work when you use offset.

If we want to use number formatter directly, we need to pass format options to plural as well.

const _plural = type => (i18n: any) => ({
value,
offset = 0,
locales = i18n.locales || i18n.language,
options,
Copy link
Contributor

Choose a reason for hiding this comment

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

options should have a default value = {}

@cristiingineru cristiingineru changed the title Add support for culture Add support for locales (aka culture) Feb 28, 2018
@@ -11,21 +13,30 @@ type PluralForms = {

type PluralProps = {
value: number,
offset?: number
offset?: number,
locales?: any,
Copy link
Author

Choose a reason for hiding this comment

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

I'm pretty sure this type should be:

locales?: ?string | string[],

but flow is complaining that null or undefined and array are incompatible with string. I don't see where the error is because locales is declared with the same type everywhere ?string | string[].

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it array? It always has to be just a single locale, right?

Copy link
Author

Choose a reason for hiding this comment

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

Intl accepts a string or an array to let the user provide a fallback in case the desired locale is not available. The first item is the number one priority, the second item is number two priority...

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that Flow can't handle default values inside object destructuring for union types. It fails also when I set format type (formerly options) to NumberFormat with default value {}. I moved default value declaration inside function (which is what Babel does anyway) and linting works fine.

@cristiingineru
Copy link
Author

Hi @tricoder42, overall what do you think about the changes? Is anything missing, anything that needs to be refactored?

@tricoder42
Copy link
Contributor

@cristiingineru I'm gonna take a look this week, I promise! I've just returned from vacation and hopefully should have more time now.

@tricoder42
Copy link
Contributor

Hey @cristiingineru, I'm finally merging this PR. Thank you very much again for writing it and sorry for such a long delay. I fixed linting for Locales (string | string[]) and renamed options to format in plural to keep it consistent with number and date. Otherwise it looks super awesome.

Final fixes are being made in #249

@tricoder42 tricoder42 closed this Jul 23, 2018
@Photonios Photonios deleted the culture branch March 6, 2019 10:50
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.

None yet

3 participants