Skip to content

feat: add plural support #284

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

jkalberer
Copy link

Internal Release Notes

This PR does a few things

  1. Support count and ordinal for i18n t function. This does not add support on Trans components because there wasn't a good example on what properties to set to support defaults
  2. Remove the opinionated logic around ***_one derived keys. These were being set as "not derived" and given specific logic to populate them with default values. This doesn't make a lot of sense since we generally will want to add values to all derived keys or none of them.
  3. Derived keys no longer use key instead of cleanKey when setting the default value. This means that you'll end up with keys/value mapping
"key_one" => "key"
"key_other" => "key"

instead of

"key_one" => "key_one"
"key_other" => "key_other"

I can't think of any case where I'd want to map the key to value like this. It's better to use the key as the fallback because if you have keyAsDefaultValue set, you want the keys to actually work as translations.

Validation Steps

yarn run test

@jkalberer
Copy link
Author

This resolves #207 and #167

Copy link
Owner

@gilbsgilbs gilbsgilbs left a comment

Choose a reason for hiding this comment

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

Thank you for this work. The PR is not too large so that's fine, but I'd appreciate more atomic PRs in the future, because my brain is no longer "hot" on that project. Anyways.

Remove the opinionated logic around ***_one derived keys. These were being set as "not derived" and given specific logic to populate them with default values. This doesn't make a lot of sense since we generally will want to add values to all derived keys or none of them.

I don't want to bikeshed this and I don't have strong feeling against it, but as it is sort of a breaking change (arguably one I'd be fairly confident making without a major version bump), I'd prefer stronger incentives. I can't say I'm very convinced by the "it clearly doesn't make sense because generally <some projection on what people are doing>" take…

Why did you implement it? Did it make code easier to write? Do you consider the old behavior a bug? I don't really see what value this change brings.

@@ -156,8 +158,9 @@ export function computeDerivedKeys(
locale,
// TODO: a comment hint should allow to override cardinality/ordinality.
// It defaults to cardinal, but this is not correct.
Copy link
Owner

Choose a reason for hiding this comment

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

This comment should be amended accordingly.

@@ -1,5 +1,6 @@
/.yarn/
/.pnp*
/.vscode/
Copy link
Owner

Choose a reason for hiding this comment

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

Please, keep this in your local ignore config.

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.

2 participants