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

Default to English translation if translation key is missing #1005

Merged
merged 3 commits into from
Jan 22, 2019

Conversation

OtterleyW
Copy link
Contributor

@OtterleyW OtterleyW commented Jan 18, 2019

If the locale in config.js is different than en we compare the selected translation to default en.json file. Translations are compared in the same way that it's done in Translations CLI.

After this update, new translation keys will not be added to other translation files with English default texts. We keep providing translations in our supported languages but they might not be up to date all the time. This means if you want to update your translations beforehand or use your own translations file, you can use translation CLI to check if there are translations missing.

src/app.js Outdated
@@ -9,7 +9,7 @@ import 'react-dates/initialize';
import Helmet from 'react-helmet';
import { BrowserRouter, StaticRouter } from 'react-router-dom';
import { Provider } from 'react-redux';
import mapValues from 'lodash/mapValues';
import { difference, mapValues } from 'lodash';
Copy link
Contributor

@Gnito Gnito Jan 21, 2019

Choose a reason for hiding this comment

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

Don't export anything directly from Lodash.
It is quite a big library and since we use a very small portion of functions it provides, we save ~15 KB of gzipped javascript if we don't import it directly.

src/app.js Outdated
const object = {};
object[key] = defaultMessages[key];
return Object.assign({}, toAdd, object);
}, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Use arrow function instead of anonymous function declaration and I think object spread is easier to understand than Object.assign.
With those changes this can be turned to oneliner:

const addedKeys = diff.reduce((toAdd, key) => ({ ...toAdd, [key]: defaultMessages[key] }), {});

or if you prefer explicit returns:

const addedKeys = diff.reduce((toAdd, key) => {
  return { ...toAdd, [key]: defaultMessages[key] };
}, {});

Although, this is probably the easiest to understand:

  const addDefaultTranslation = (translations, missingKey) => ({ ...translations, [missingKey]: defaultMessages[missingKey] });
  const addedKeys = diff.reduce(addDefaultTranslation, messagesInLocale);

Copy link
Contributor

@Gnito Gnito Jan 21, 2019

Choose a reason for hiding this comment

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

Actually, I'd suggest this:

const addMissingTranslations = (sourceLangTranslations, targetLangTranslations) => {
  const sourceKeys = Object.keys(sourceLangTranslations);
  const targetKeys = Object.keys(targetLangTranslations);
  const missingKeys = difference(sourceKeys, targetKeys);

  const addMissingTranslation = (translations, missingKey) => 
    ({ ...translations, [missingKey]: sourceLangTranslations[missingKey] });

  return missingKeys.reduce(addMissingTranslation, targetLangTranslations);
};

const isDefaultLanguageInUse = config.locale === 'en';
const messages = isDefaultLanguageInUse
  ? defaultMessages
  : addMissingTranslations(defaultMessages, messagesInLocale);

Copy link
Contributor

Choose a reason for hiding this comment

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

(btw. I didn't test that ^)

src/app.js Outdated
// Flex template application uses English translations as default.
// If translation key is missing from e.g. fr.json corresponding key
// will be added to messages from en.json to prevent missing translation key errors.
import defaultMessages from './translations/en.json';
Copy link
Contributor

@Gnito Gnito Jan 21, 2019

Choose a reason for hiding this comment

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

Now that I read these comments, I think that these lines (20-34) should be reordered a bit:

// Flex template application uses English translations as default.
import defaultMessages from './translations/en.json';

// If you want to change the language, change the imports to match the wanted locale:
//   1) Change the language in the config.js file!
//   2) Import correct locale rules for React Intl library
//   3) Import correct locale rules for Moment library
//   4) Use the `messagesInLocale` import to add the correct translation file.

// Step 2:
// Import locale rules for React Intl library 
import localeData from 'react-intl/locale-data/en';

// Step 3:
// If you are using a non-english locale with moment library,
// you should also import time specific formatting rules for that locale
// e.g. for French: import 'moment/locale/fr';

// Step 4:
// If you are using a non-english locale, point `messagesInLocale` to correct .json file
import messagesInLocale from './translations/en.json';

// If translation key is missing from `messagesInLocale` (e.g. fr.json), 
// corresponding key will be added to messages from `defaultMessages` (en.json) 
// to prevent missing translation key errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also docs should reflect this step-by-step model ^

Copy link
Contributor

@Gnito Gnito left a comment

Choose a reason for hiding this comment

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

I'd refactor this a bit more.

@OtterleyW OtterleyW merged commit 6a5b3af into master Jan 22, 2019
@OtterleyW OtterleyW deleted the default-to-en-translation branch January 22, 2019 13:34
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.

2 participants