Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Chinese translations and intl demo #382

Open
wants to merge 18 commits into
base: feature-i18n
Choose a base branch
from

Conversation

zianke
Copy link

@zianke zianke commented Apr 23, 2019

Related Issue:
66

Summary:
We have completed the Chinese translations and put all the translation-related files in the locale directory, as we've discussed in the issue. These files are connected to the components by intl.reducer.js and react-intl-redux.

The next step will be to modify all of the components with messages. We've already done this part with the IntroScreen and the CreateNewProjectWizard/BuildPanel component. You can take them as a simple demo. This pull request will keep being updated in the following days to include the change of other components.

By the way, do you think we should submit the pull request to the i18n-app-menu-item branch? Or can you create another branch (e.g. i18n) for the following updates?

Screenshots/GIFs:
screenshot2

@AWolf81
Copy link
Collaborator

AWolf81 commented Apr 23, 2019

Hi @zianke:
Thanks for the update.
After briefly looking at the code it looks great so far. I'll do a detailed review later.

I have just some questions/comments:

  • intl.reducer I think it's OK to have it & do it as you have implemented it - just wondering what benefit we would have if we would use intlReducer from react-intl-redux e.g. dispatch updateIntl({...}) - is this only doing the state updating?
  • setState warning after initial load (see screenshot below) - can't setState on an unmounted component. Not sure why we're getting that message. Update OK, this is not related to i18n and can be fixed by adding _isMounted in Initialization.js component to avoid setting state on unmounted component. Like mentioned in this post. I have added it to the feature-i18n branch.
    grafik
  • You have changed the currently selected lanuage from app-settings.reducer to intl.reducer. I think that's working but I'd like to have the currently selected locale in app-settings reducer to keep every setting there. I understand why you did it as it's easier to get the initialLocale. We can improve this with a app-loaded.saga later - it's OK to keep it as it is.

Sure, I can create a new branch i18n based on i18n-app-menu-item so we can merge the next updates there. Maybe we can also change the target of this PR to the new branch - I'll check if it's possible to change.

@AWolf81 AWolf81 changed the base branch from i18n-app-menu-item to feature-i18n April 23, 2019 12:17
@@ -62,6 +62,8 @@
"ps-tree": "1.2.0",
"react-beautiful-dnd": "10.1.1",
"react-custom-scrollbars": "4.2.1",
"react-intl": "^2.8.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Please pin the version by removing the ^ for react-intl and react-intl-redux.

Copy link
Collaborator

@AWolf81 AWolf81 left a comment

Choose a reason for hiding this comment

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

LGTM.

Just smaller things open (see other comment & inline comments). I haven't read every copy text but I will check them once I'm doing the German translation.
Once your Chinese translation is ready I think we can merge it to master. Should we write a how to i18n documentation so we're having a guide for other translations?
I'll add a todo list to the i18n issue so we're having a reminder there and more details to the current status.

Your screenshot is a bit confusing as the ProjectPage / CreateNewProjectWizard is not available in i18n branch but it's probably available in your WIP locally.

},
};

const definedMessages = defineMessages(messages);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think we can directly do a default export as definedMessages is only used for exporting (also applies to the other .message files)

Suggested change
const definedMessages = defineMessages(messages);
export default defineMessages(messages);

file: {
label: {
id: 'applicationMenu.file.label',
defaultMessage: '&File',
Copy link
Collaborator

@AWolf81 AWolf81 Apr 23, 2019

Choose a reason for hiding this comment

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

Is the accelerator working? I wasn't sure but it's working as expected. By pressing alt key on Windows it will display the acceleator by unterlining the Letter F in the menu bar.
grafik

@AWolf81
Copy link
Collaborator

AWolf81 commented May 8, 2019

@zianke any progress on this? What do you think is this ready for merge to branch feature-i18n?
I think I can fix the one open inline comment on the branch.
We can create follow up PRs if you're having updates. I'd like to keep the branch i18n a bit longer active until we're having some languages ready.
I'd like to start with German translation and a guide for how to add more languages.

@zianke
Copy link
Author

zianke commented May 9, 2019

@AWolf81 Sorry for the delay of reply. It has been two busy weeks. Hopefully I'll have some time these days to continue on the tasks.
react-intl-redux is just a simple wrapper over react-intl to implement redux binding. For basic react-intl, we need to set locale and messages with <IntlProvider locale={...} messages={...}>. Since we have IntlProvider in src/index.js, however, it's not easy to access locale or messages and update them in src/index.js. react-intl-redux can instead read from the redux state, more specifically, the values of keys intl.locale and intl.messages. That's why I wrote a intl.reducer so that the values appear in the redux state with the specified keys instead of something like app-settings.locale, otherwise react-intl-redux cannot read them correctly. It's possible to move the locale value back to app-settings.reducer, but we may need to change something on react-intl-redux. I'm currently reading the source code of react-intl-redux to see if we could customize the keys from which locale and messages are read from redux.
Another important question to consider is how to implement internationalization in non-component code. For example, there are some hard-coded strings in types.js. This has been a popular issue of react-intl, with many discussions here. You may want to have a look first. Once we find a best solution to this problem, I'll update a non-component js file in this pull request to reflect it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants