Prototype with proposed i18n architecture #1478
Conversation
Adding the i18n file.
i18n functions in several entries.
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. |
There was a problem hiding this comment.
This is so exciting and is looking very good!
As this PR is being merged into develop it'll get deployed to production when we do the next release. So it's important that anything that isn't finished is hidden behind a feature flag (using an environment variable). Nav.jsx has a few examples where this is done.
It's a bit tricky for this work but I'd suggest:
- we figure out a nice "loading" screen to use so users don't see the white page and "loading translations"
- we hide the language switcher so users only see the english default language
That way we can continue with this work in small PRs but users don't see any changes.
| ); | ||
| } | ||
|
|
||
| renderLanguageMenu(navDropdownState) { |
There was a problem hiding this comment.
Until we're ready to make this live for users, we should hide the language switcher behind an environment variable. You can use other examples in this component as an example.
client/components/Nav.jsx
Outdated
| > | ||
| New | ||
| </button> | ||
| <Translation ns="Menu"> |
There was a problem hiding this comment.
What do you think about using the withTranslation HoC instead of this Translation component?
I think it would be clearer to see what's actually being changed in these files as we'd wrap Nav in withTranslation('Menu')(Nav) at the bottom of the file and then use this.props.t('key').
This would avoid indenting all the files and would be less changes in the PR diff views.
Debug to false
comments in server and change of path in server.js
comments in server and change of path in server.js Uses WithTranslation In Nav
|
Amazing work! This is so exciting ✨ |
Warning if unsaved translated
Expanded label for languages
# Conflicts: # client/components/Nav.jsx # client/i18n.js # client/index.jsx # client/modules/IDE/components/Toast.jsx # package-lock.json # server/server.js
| @@ -79,6 +79,7 @@ app.options('*', corsMiddleware); | |||
| app.use(Express.static(path.resolve(__dirname, '../dist/static'), { | |||
| maxAge: process.env.STATIC_MAX_AGE || (process.env.NODE_ENV === 'production' ? '1d' : '0') | |||
| })); | |||
| app.use('/translations', Express.static('translations/locales/')); | |||
There was a problem hiding this comment.
Rather than serve the translations statically, would it instead be better to bundle them in with the webpack build? Unfortunately, serving the files this way breaks the application in production, because they are not copied in the Docker build. On the staging server, fetching the translations is returning a 404 error.
cc @andrewn
There was a problem hiding this comment.
Or, the Dockerfile needs to be updated to copy the translations.
There was a problem hiding this comment.
Or or, the webpack prod configuration needs to be updated so that it copies the files into dist/static
There was a problem hiding this comment.
Ah, I didn’t realise that, sorry.
The translations shouldn’t be in the bundle as we don’t want users to have to download languages they won’t use. But I guess copying the translations into dist/static would be simplest.
There was a problem hiding this comment.
No worries at all! Let me know if y'all need any help with this.
Fixes #1434 , Fixes #1447
I have verified that this pull request:
npm run lint)Fixes #123