-
Notifications
You must be signed in to change notification settings - Fork 379
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
Conversation
What do you think? I'm open to suggestions here |
Of course, having a default complicated the things a little bit, but hopefully not that much for the user. The priority of using Taking into account The There was a But, not sure about the |
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) |
There was a problem hiding this comment.
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:
js-lingui/packages/core/src/context.js
Line 14 in 0d0aa92
return norm.map(m => (isString(m) ? m.replace("#", value) : m)) |
There was a problem hiding this comment.
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.
packages/core/src/select.js
Outdated
const _plural = type => (i18n: any) => ({ | ||
value, | ||
offset = 0, | ||
locales = i18n.locales || i18n.language, | ||
options, |
There was a problem hiding this comment.
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 = {}
@@ -11,21 +13,30 @@ type PluralForms = { | |||
|
|||
type PluralProps = { | |||
value: number, | |||
offset?: number | |||
offset?: number, | |||
locales?: any, |
There was a problem hiding this comment.
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[]
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Hi @tricoder42, overall what do you think about the changes? Is anything missing, anything that needs to be refactored? |
@cristiingineru I'm gonna take a look this week, I promise! I've just returned from vacation and hopefully should have more time now. |
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 Final fixes are being made in #249 |
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:
locales
andoptions
- be further exposed through the js-lingui API?locales
be further exposed aslocales
orculture
? Other languages are using culture, or cultureNames...i18n
?