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

React native docs #243

Merged
merged 3 commits into from
Jul 22, 2018
Merged

React native docs #243

merged 3 commits into from
Jul 22, 2018

Conversation

vonovak
Copy link
Collaborator

@vonovak vonovak commented Jul 11, 2018

No description provided.

Copy link
Contributor

@tricoder42 tricoder42 left a comment

Choose a reason for hiding this comment

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

This is nice so far! Great work!


This was easy. Now, the next step is to translate the ``title`` prop of the ``Button`` component. But wait a sec, the button expects to receive a ``string``, so we cannot use the ``<Trans>`` component here! Also notice the same holds also for the ``Alert.alert`` call.

At this point, we need to turn our attention to the ``@lingui/core`` package, namely the ``setupI18n`` method which can give us an object that we can use like this: ``i18n.t`this will be translated``` and the result of such call is a string. Let's get to it!
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be actually done using withI18n decorator which injects i18n object to props. I can't believe I haven't mentioned in React docs... I have to update it too. The only mention about this (with short example) is in React API reference. After that, everything is the same.

Copy link
Collaborator Author

@vonovak vonovak Jul 12, 2018

Choose a reason for hiding this comment

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

This should be actually done

sorry, I don't exactly know what you have in mind. Can you take a look at the final code here and perhaps make a PR?

Or do you have this in mind:

<Button onPress={markAsRead} title={i18n.t`mark messages as read`} />

to become

<Button onPress={markAsRead} title={this.props.i18n.t`mark messages as read`} />

?

My idea is to show both translation in react (Trans/ this.props.i18n) and outside of react (i18n.t)

@vonovak vonovak changed the title [wip] React native docs React native docs Jul 13, 2018
@vonovak
Copy link
Collaborator Author

vonovak commented Jul 13, 2018

this should be good enough as the first version of the rn docs, can you add your review? Thanks. Btw. the jsx highlighter has some problems with things like "i18n.tblah blah".

also, what branch should I target?

Copy link
Contributor

@tricoder42 tricoder42 left a comment

Choose a reason for hiding this comment

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

Hey, this is nice work! Use stable-2.x as a base branch and don't worry about conflicts. I'll resolve them manually when the PR is ready.

},
});
...
<Button onPress={markAsRead} title={i18n.t`mark messages as read`} />
Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean here is that you don't need to use setupI18n when you're working with translations inside your components. You need to work with standalone i18n object only outside components, e.g.: in redux-saga actions, etc.

In components, you're better off to wrap you component in withI18n decorator and then you injected i18n prop:

import { withI18n } from '@lingui/react'

function MarkAsRead({ i18n, markAsRead }) {
  return <Button onPress={markAsRead} title={i18n.t`mark messages as read`} />
}

// withI18n expects options as a first argument,
// so we need to call it with empty parenthesis to get the actual HOC
export default withI18n()(MarkAsRead)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, gotcha. Then my question is why?

Also, most apps need both - accessing the i18n in components and outside of them - so this approach, if it is valid (?) need to be documented somewhere. I'm okay with using just this.props.i18n.t here because we only deal with components, but then we need to do another guide that will cover using both using i18n.t and this.props.i18n.t and explain why using this.props.i18n.t in components is better that just i18n.t because I'm a lazy person and don't understand why I should use something from props if I can just import a function (i18n.t) and use it everywhere. Hope you understand me. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I do. The main reason is re-rendering. When you directly call a function inside a component, the component itself won't re-render when the return value changes because of outside effects.

function TranslatedComponent() {
   return <span>{i18n.t`Hello World`}</span>
}

When you render this component, it will show translation for Hello World. However, when you change the active language, this component will not re-render. This was common issue in react-intl and the only known workaround is to pass key={language} to top-most component and force complete re-render of the whole app.

On the other hand, when you use withI18n decorator:

const TranslatedComponent = withI18n()(({ i18n }) => {
   return <span>{i18n.t`Hello World`}</span>
})

Now withI18n HOC is responsible not only for injecting i18n object, but also for forcing re-render when language or message catalog changes.

Other reason I might thing off: when you use i18n directly, you're using global variable, which makes component very hard for testing. I always use functions/variables passed as props instead of global ones.

What do you think? I'm really open for discussion here as there should be one and preferably only one way to do it.

Copy link
Collaborator Author

@vonovak vonovak Jul 16, 2018

Choose a reason for hiding this comment

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

okay, that sounds good. So the recommended approach for projects where both component and non-component code needs to be translated (all normal projects :D) is to
1 . call setupI18n to get the i18n object,
2 . pass it to i18nProvider and use this.props.i18n in components
3 . use the original i18n object outside of components

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!

In my project, I don't need i18n outside components. I'm working on React/Apollo app and everything is either a Component (React) or HOC (Apollo containers). AFAIK even redux-form has access to prop in validation function (my previous project) so I always considered using standalone i18n object as something special :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, so let me rephrase that to "all normal react native projects" :)

I'll apply the suggested changes and push them this week. Where should I document the 1, 2, 3 steps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! I haven't worked with react-native yet.

I think this section is perfect place to document it. Later we might write a standalone guide to further explain differences and reasoning. For tutorial purposes, let's keep it simple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One more question: we initially need to call setupI18n to get the i18n object. Later if I want to change the language, I could create a new i18n object by again calling setupI18n, or I can call i18n.load and i18n.activate like here. Which way should be documented? I kinda like the second one better but not sure if that api is stable..?


The important point here is that the sentence isn't broken into pieces but remains together - that will allow the translator to deliver a quality result.
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this! That's exactly the point.

@tricoder42
Copy link
Contributor

tricoder42 commented Jul 19, 2018 via email

@vonovak
Copy link
Collaborator Author

vonovak commented Jul 20, 2018

I’m not sure a new i18n would propagate cleanly

yes, exactly.

So, the docs should be looking good now! I just need to update the example app and link it at the end of the tutorial.

@vonovak
Copy link
Collaborator Author

vonovak commented Jul 20, 2018

also, this probably closes #176

wip 2

add image

fix typo

use withI18n

add demo link
@codecov
Copy link

codecov bot commented Jul 21, 2018

Codecov Report

Merging #243 into stable-2.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           stable-2.x     #243   +/-   ##
===========================================
  Coverage       91.62%   91.62%           
===========================================
  Files              41       41           
  Lines             931      931           
===========================================
  Hits              853      853           
  Misses             78       78

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85eb87a...a7580bb. Read the comment docs.

@vonovak
Copy link
Collaborator Author

vonovak commented Jul 21, 2018

@tricoder42 I rebased this onto stable-2.x so there are no conflicts, and updated the example app, so this is ready for another review (perhaps ready for merging :) ).

@tricoder42 tricoder42 merged commit f0ebd7d into lingui:stable-2.x Jul 22, 2018
@tricoder42
Copy link
Contributor

This is nice! I'm gonna to merge it now. Thank you for you help! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants