App translations framework #74
Conversation
activeLanguage state doesn't persist yet
Goes with server changes that removed mongoose-intl package
| @@ -88,7 +90,8 @@ export default class App extends Component { | |||
| const AppContainer = createAppContainer(AppNavigator); | |||
|
|
|||
| return ( | |||
| <Provider store={store}> | |||
| <Provider store={store}> | |||
There was a problem hiding this comment.
Not a bit deal, but there are some whitespace issues here that would probably be fixed with our linter.
| @@ -102,7 +104,36 @@ class SettingsPage extends Component { | |||
| <Content style={styles.content}> | |||
| {userBlock} | |||
| <View style={styles.view}> | |||
| <H3>Device Settings</H3> | |||
| <Translate> | |||
| {({ translate }) => ( | |||
There was a problem hiding this comment.
Another whitespace issue, but it's fine if you don't feel like fixing it.
| /> | ||
| </Item> | ||
| <Item> | ||
| <Label>Description (ES)</Label> |
There was a problem hiding this comment.
I wonder if there's a way we can encapsulate the idea of repeating the same form element multiple times for each language option in a component, like <LocalizedInputs> that takes the attribute it's affecting, like description in this case. Not something we need to do right now, but if we need to do a lot of this kind of thing, we should consider it later.
| import { StyleSheet, View } from "react-native"; | ||
| import { Translate, withLocalize, getLanguages, getActiveLanguage, setActiveLanguage |
There was a problem hiding this comment.
We do not use Translate, getActiveLanguage, or setActiveLanguage in this component, so we can remove those imports.
| @@ -112,9 +112,9 @@ class EventsMap extends Component { | |||
| const { | |||
| props: { events: fetchedEvents } | |||
| } = this; | |||
| const previousEventIds = prevProps.events.map(event => event.id); | |||
| const previousEventIds = prevProps.events.map(event => event._id); | |||
There was a problem hiding this comment.
Why did we transition to _id here?
There was a problem hiding this comment.
The underscore is actually a default of the server, and while we had a server-side i18n library hooked up I took out the underscore on the id to keep that library from getting confused. Now that the library is taken out, though, none of that is necessary, so I changed it back.
| @@ -112,7 +111,7 @@ class SettingsPage extends Component { | |||
| <Form> | |||
| <Item> | |||
| <Label><Translate id="settings.language" /></Label> | |||
| <Text>{this.props.currentLanguage}</Text> | |||
| <Text>{this.props.activeLanguage.name}</Text> | |||
There was a problem hiding this comment.
Where are we getting activeLanguage from?
There was a problem hiding this comment.
activeLanguage is a prop from react-localize-redux, along with the translate() function and languages object. These props are added to a component by wrapping the component in withLocalize().
| @@ -2,6 +2,7 @@ | |||
| import React, { Component } from "react"; | |||
| import { StyleSheet, Text, View } from "react-native"; | |||
| import { connect } from "react-redux"; | |||
| import { Translate, withLocalize, setActiveLanguage } from "react-localize-redux"; | |||
There was a problem hiding this comment.
It doesn't look like Translate is being used in EventsMap, so we can remove that import.
| } | ||
| // Save settings in redux store | ||
| await this.props.saveDevice(deviceSettings); | ||
| } |
There was a problem hiding this comment.
Why are we saving settings about the language and device into Redux from the EventsMap? If it's because we need to grab settings from local storage and configure the app on first load or something, is there some way we can do that more globally, so that EventsMap isn't responsible for it?
There was a problem hiding this comment.
That's a good question-- I put all of the first-load store fetching stuff in EventsMap because that's the first screen users land on, and putting stuff in App.js didn't work since that's where the framework-level stuff (Redux, React Navigation, NativeBase, react-localize-redux) hooks in, and I wasn't sure where else to put it because React Navigation is in there and the root component this logic should be in wasn't super clear. Open to suggestions!
There was a problem hiding this comment.
Gotcha! It's fine to leave it for now if we want to just keep moving, but one suggestion would be to create a new component underneath App.js wrapping everything else that just does the saving on componentDidMount and then renders children. Or a Higher Order Component that wraps EventsMap and does what I just described.
I'm just imagining that, if we ever change the first screen that appears, then we'll have to move this logic, or duplicate it if there's a choice for what screen shows up first somehow. Like I said, not a huge deal right now, though. Perhaps this screen will always be first.
There was a problem hiding this comment.
Ok! Going to try to add a higher-order wrapper component to hook into in the next PR.
There was a problem hiding this comment.
I merged this is already, but FYI I just came across this in the React Navigation docs, which I might try: https://reactnavigation.org/docs/en/localization.html
| // set active language in device (redux store) | ||
| await this.props.saveDeviceLanguage(change); | ||
| // save device settings to asyncstorage (device storage) | ||
| await deviceServices.set(this.props.device); |
There was a problem hiding this comment.
Have we not set up device storage as where we store redux data yet? I thought we had in some previous work, but I could be misremembering. If we have done that, do we have to set both redux and device storage separately?
There was a problem hiding this comment.
We're saving to/fetching from device storage one item at a time, and whenever the user makes a change to a given item, that item has to be changed in both the Redux store and device storage. Not super efficient code-wise, but a) I figured if the Redux store gets big it wouldn't be efficient to save the whole thing to device storage every time the user makes a change, and b) If we want to make async calls (like any call to asyncStorage) in Redux actions, I believe we need to add redux-thunk or something. (But I just noticed that getEvents in EventsMap appears to work even though it's making an API call with an async dispatch function sans thunk.)
There was a problem hiding this comment.
Sure that's fine! I only ask because if another dev tries to build similar work, I could see them missing this and forgetting to save to one of the places. I'm just looking for ways to make it was easy as possible for future contributors to expand on our patterns.
And yeah I don't think you need to use redux-thunk for async calls. I'm pretty sure that library just makes it easier to make API calls, removing some of the boilerplate. But it's not required.
There was a problem hiding this comment.
I do believe that we're already saving all of the Events to Redux already, and that's definitely going to be the vast majority of the size of the Redux store. Maybe we don't worry about whether the Redux store is too big until that actually happens, and so go for the easiest to follow code rather than the most optimized code at this stage?
It's fine to retain this approach, though, especially if we encapsulate saving to both of those places into some kind of utility so we don't have to remember to save to both every time.
There was a problem hiding this comment.
Ok, I'll do some refactoring of this in the next PR-- I agree that we need more utilities for hooking up Redux and the async store, and if I don't have to add another library just to handle async calls from Redux dispatch, so much the better.
Closes #63, #73 & #75 .