-
Notifications
You must be signed in to change notification settings - Fork 54
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
chore(react-i18n): update react-i18next #1344
Conversation
…i into jare/chore/update-react-i18next
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.
There are linter errors too.
@@ -87,7 +87,7 @@ ActionSplitDropdown.propTypes = { | |||
|
|||
ActionSplitDropdown.defaultProps = { | |||
items: [], | |||
t: getDefaultTranslate, | |||
t: getI18n().t.bind(getI18n()), |
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.
Is it possible to avoid to write this bind() everywhere ?
For example in translate.js
, the getDefaultTranslate is not used anymore with your changes, you can replace it with return getI18n().t.bind(getI18n())
so in components we can white the defaultProps t: getDefaultTranslate()
(same for all components)
@@ -1,7 +1,7 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|||
exports[`ActionBar should render a btn-group 1`] = ` | |||
<ActionBar | |||
<I18n |
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.
This makes the snapshot ko.
Now instead of having an ActionBar
, we have an I18n
. Perhaps you should import the component with the translate HOC for the tests.
(same for all components)
debug: false, | ||
wait: true, // globally set to wait for loaded translations in translate hoc | ||
}); | ||
// i18n.init({ |
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.
Remove all that is it is not needed anymore.
Remove the import of I18N_DOMAIN_COMPONENTS
too
@@ -92,7 +94,7 @@ export class DeleteResource extends React.Component { | |||
validateAction={validateAction} | |||
> | |||
<div> | |||
<Trans i18nKey={i18nKey}> | |||
<Trans i18nKey={i18nKey} parent="div"> |
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.
Even with the doc link I don't understand why this is needed.
If it add a div to wrap the result, the parent div
can be removed right ?
…i into jare/chore/update-react-i18next
…i into jare/chore/update-react-i18next
What is the problem this PR is trying to solve?
The last version of react-i18next is 7.x.x. Our version is 5.x.x
Some change with the last version:
translate change the prior to get the i18n. The props are used by default and no the context. The priority is props > context > getI18n. So, now to use UI without i18n, we pass a default i18n by the method setI18n by import 'translate'. If the component have no i18n context so it uses the getI18n to get it.
The snapshot are changed because react-i18n wrap the translate with a I18n component.
What is the chosen solution to this problem?
Please check if the PR fulfills these requirements
[x] This PR introduces a breaking change
Update react-i18next to the last version to get the method getI18n, setI18n