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

feat: Default translation values #86

Merged
merged 10 commits into from
Feb 8, 2022
Merged

feat: Default translation values #86

merged 10 commits into from
Feb 8, 2022

Conversation

maxwoedl
Copy link
Contributor

@maxwoedl maxwoedl commented Feb 5, 2022

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
Copy link

vercel bot commented Feb 5, 2022

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
Copy link
Contributor Author

maxwoedl commented Feb 5, 2022

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
Copy link
Owner

@amannn amannn left a comment

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.

@@ -39,6 +40,9 @@ type Props = {
* afterwards the current date will be returned continuously.
*/
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
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
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 :)

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

Copy link
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
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
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.

packages/website/pages/docs/usage/configuration.mdx Outdated Show resolved Hide resolved
@maxwoedl maxwoedl changed the title feat: Default Rich Text Elements feat: Default translation values Feb 7, 2022
@@ -272,7 +272,7 @@ describe('formatNumber', () => {

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
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
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
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.

@@ -149,6 +149,7 @@ t('selectordinal', {year: 11});
## 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
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.

Copy link
Owner

@amannn amannn left a comment

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
Copy link
Contributor Author

maxwoedl commented Feb 8, 2022

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