Skip to content

feat: Default translation values - #86

Merged
amannn merged 10 commits into
amannn:mainfrom
maxwoedl:main
Feb 8, 2022
Merged

feat: Default translation values#86
amannn merged 10 commits into
amannn:mainfrom
maxwoedl:main

Conversation

@maxwoedl

@maxwoedl maxwoedl commented Feb 5, 2022

Copy link
Copy Markdown
Contributor

To achieve consistent usage and styling of rich text elements, I have included an option to define global defaults for rich text elements. The default will be overridden by a local definition of an element.

This approach is the same as the one from react-intl.

@vercel

vercel Bot commented Feb 5, 2022

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/amann/next-intl/52DRkv9ncDnYUVNock6ANqdtmGrw
✅ Preview: https://next-intl-git-fork-maxwoedl-main-amann.vercel.app

@maxwoedl

maxwoedl commented Feb 5, 2022

Copy link
Copy Markdown
Contributor Author

BTW is one of use-intl's tests broken due to a spelling mistake (plural and singular), I can also create a PR to fix it if you like.

@maxwoedl maxwoedl changed the title Default Rich Text Elements feat: Default Rich Text Elements Feb 5, 2022

@amannn amannn left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks a lot for looking into this @maxwoedl! I hope you found your way around in the code base, your change is really well done! 👏

I've added some minor comments, but I think we can get this in.

Comment thread packages/use-intl/src/IntlProvider.tsx Outdated
now?: Date;
/** Global rich text elements for consistent styling or usage of rich text tags.
* Will be overidden by locally provided elements. */
defaultRichTextElements?: RichTranslationValues;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

At first I thought we might need to restrict this to only accept functions and no TranslationValue to avoid abusing the config option. But now I'm wondering if we should expand this to defaultTranslationValues?: RichTranslationValues.

I mean the primary use case is definitely to supply rich text elements, but this could be convenient to provide some common values that are often used. You can use state management solutions for this, but maybe it could come in handy for data that is only used in messages.

What's your opinion here? I tend to go for defaultTranslationValues to give the user more capabilities since we're already adding this feature.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I had the same thought the day after the PR. I am also in favour of this solution and not just the rich text elements :)

Comment thread packages/website/pages/docs/usage/configuration.mdx Outdated
Comment thread packages/website/pages/docs/usage/configuration.mdx Outdated
Comment thread packages/use-intl/src/useTranslations.tsx Outdated
<App />
</NextIntlProvider>
```

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

BTW is one of use-intl's tests broken due to a spelling mistake (plural and singular), I can also create a PR to fix it if you like.

Which is the test you're seeing problems with? Seems like CI passes. I'd be curious if it's a problem related to the environment. You might need node>=14, this is currently not enforced.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

use-intl/test/useIntl.test.tsx:274:29

It's simply a spelling mistake, the expected value is "Invalid currency codes" (plural) where as the recieved value is "Invalid currency code" (singular).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think if you run git revert 34d9c05 in your branch that should do the trick.

Comment thread packages/website/pages/docs/usage/configuration.mdx Outdated
@maxwoedl maxwoedl changed the title feat: Default Rich Text Elements feat: Default translation values Feb 7, 2022
Comment thread packages/use-intl/test/useIntl.test.tsx Outdated
const error: IntlError = onError.mock.calls[0][0];
expect(error.message).toBe(
'FORMATTING_ERROR: Invalid currency codes : unknown'
'FORMATTING_ERROR: Invalid currency code : unknown'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hmm, locally this is indeed "codes" for me with node v14.19.0. Seems like on CI there's the same result. Which node version are you using where you're seeing "code"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am using node v16.13.1, so you were correct about the node version, sorry for "fixing" the test, i will undo it :)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

No problem! An upgrade to node v16 would definitely be interesting, but I'm not sure yet if there are other CI-related things to update then so that can be done at a later point for sure.

## Rich text

You can format rich text with custom tags and map them to React components.
If you want to use the same tag multiple times, you can configure it via the [default translation values](/docs/usage/configuration#default-translation-values).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nit: Can we move this after the code snippet? I feel like it could be helpful to first finish introducing the concept and then providing supplemental information like the global config. It might also help a bit with discovery, I tend to skip directly to the code examples when reading a section and expect advanced information to follow afterwards.

@amannn amannn left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Really great PR, thank you so much @maxwoedl! 🙌

@amannn
amannn disabled auto-merge February 8, 2022 07:30
@amannn
amannn merged commit 0ed5e70 into amannn:main Feb 8, 2022
@maxwoedl

maxwoedl commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

Really great PR, thank you so much @maxwoedl! 🙌

Was a pleasure to work with you @amannn! ☺️

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