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.

Sign up for GitHub

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

Fix conflicting strings issue in translations #917

Merged
merged 5 commits into from
Aug 28, 2018
Merged

Fix conflicting strings issue in translations #917

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9cf0cc8
Fix conflicting strings issue in translations
notlmn Aug 26, 2018
dae9e44
Preserve structure of `customTranslations`
notlmn Aug 27, 2018
c608bbc
Use `deepmerge` to merge whole of `localized-strings`
notlmn Aug 27, 2018
077f9c8
Simplify and make deep property access on an object safe
notlmn Aug 27, 2018
cf1405c
Fix deep property accessor and rename it to idx
notlmn Aug 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions docs/guides-translation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,16 @@ If you want to add additional custom translation strings, or override any of the
```json
{
"localized-strings": {
"id": "string",
"id2": "string2"
"docs": {
"id": {
"title": "string1",
"sidebar_label": "string2"
},
"version-0.0.1-id": {
"title": "string3",
"sidebar_label": "string4"
}
}
},
"pages-strings" : {
"id3": "string3",
Expand Down
10 changes: 6 additions & 4 deletions lib/core/DocsLayout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,17 @@ class DocsLayout extends React.Component {
const metadata = this.props.metadata;
const content = this.props.children;
const i18n = translation[this.props.metadata.language];
const id = metadata.localized_id;
const defaultTitle = metadata.title;
let DocComponent = Doc;
if (this.props.Doc) {
DocComponent = this.props.Doc;
}
const title = i18n
? translation[this.props.metadata.language]['localized-strings'][
this.props.metadata.localized_id
] || this.props.metadata.title
: this.props.metadata.title;
? (i18n['localized-strings'].docs[id] &&
Copy link
Contributor

@endiliey endiliey Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Seeing this made me realize that we should create a safe getter to access deeply nested object.
We don't want to keep doing this. Example:

// access deeply nested values...
translation &&
translation[this.props.metadata.language] &&
translation[this.props.metadata.language].docs  &&
translation[this.props.metadata.language].docs[id]

We can create a small helper function (maybe in core/utils.js)

const idx = (target, path) =>
  path.reduce((obj, key) => (obj && obj[key] ? obj[key] : null), target);

So we can write

const title = idx(i18n, ['localized-strings', 'docs', id, 'title']) || defaultTitle;

instead of

const title = i18n ? (i18n['localized-strings'].docs[id] && i18n['localized-strings'].docs[id].title) || defaultTitle : defaultTitle;

WDYT ?

Copy link
Contributor Author

@notlmn notlmn Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I used almost the same method you used to access a deep property, but modified it so that the function accepts the path as a string rather than an array, because most of the libraries out there follow the same pattern.

If we need to be more safe then we can move to any library like just-safe-get.

i18n['localized-strings'].docs[id].title) ||
defaultTitle
: defaultTitle;
const hasOnPageNav = this.props.config.onPageNav === 'separate';
return (
<Site
Expand Down
5 changes: 2 additions & 3 deletions lib/core/nav/HeaderNav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,11 @@ class HeaderNav extends React.Component {
(link.blog && this.props.current.blogListing) ||
(link.page && link.page === this.props.current.id),
});
const i18n = translation[this.props.language];
return (
<li key={`${link.label}page`} className={itemClasses}>
<a href={href} target={link.external ? '_blank' : '_self'}>
{translation[this.props.language]
? translation[this.props.language]['localized-strings'][link.label]
: link.label}
{i18n ? i18n['localized-strings'].links[link.label] : link.label}
</a>
</li>
);
Expand Down
15 changes: 10 additions & 5 deletions lib/core/nav/SideNav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ class SideNav extends React.Component {
// return appropriately translated category string
getLocalizedCategoryString(category) {
const categoryString = translation[this.props.language]
? translation[this.props.language]['localized-strings'][category] ||
category
? translation[this.props.language]['localized-strings'].categories[
category
] || category
: category;
return categoryString;
}
Expand All @@ -26,16 +27,20 @@ class SideNav extends React.Component {
getLocalizedString(metadata) {
let localizedString;
const i18n = translation[this.props.language];
const id = metadata.localized_id;
const sbTitle = metadata.sidebar_label;

if (sbTitle) {
localizedString = i18n
? i18n['localized-strings'][sbTitle] || sbTitle
? (i18n['localized-strings'].docs[id] &&
i18n['localized-strings'].docs[id].sidebar_label) ||
sbTitle
: sbTitle;
} else {
const id = metadata.original_id || metadata.localized_id;
localizedString = i18n
? i18n['localized-strings'][id] || metadata.title
? (i18n['localized-strings'].docs[id] &&
i18n['localized-strings'].docs[id].title) ||
metadata.title
: metadata.title;
}
return localizedString;
Expand Down
2 changes: 1 addition & 1 deletion lib/server/readMetadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ function generateMetadataDocs() {

// metadata for english files
const docsDir = path.join(CWD, '../', getDocsPath());
let files = glob.sync(`${CWD}/../${getDocsPath()}/**`);
let files = glob.sync(`${docsDir}/**`);
files.forEach(file => {
const extension = path.extname(file);

Expand Down
42 changes: 32 additions & 10 deletions lib/write-translations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const fs = require('fs-extra');
const glob = require('glob');
const mkdirp = require('mkdirp');
const nodePath = require('path');
const deepmerge = require('deepmerge');

const readMetadata = require('./server/readMetadata.js');

Expand All @@ -34,7 +35,11 @@ const siteConfig = require(`${CWD}/siteConfig.js`);
const sidebars = require(`${CWD}/sidebars.json`);

let customTranslations = {
'localized-strings': {},
'localized-strings': {
docs: {},
links: {},
categories: {},
},
'pages-strings': {},
};
if (fs.existsSync(`${CWD}/data/custom-translation-strings.json`)) {
Expand All @@ -54,13 +59,20 @@ function execute() {
next: 'Next',
previous: 'Previous',
tagline: siteConfig.tagline,
docs: {},
links: {},
categories: {},
},
'pages-strings': {},
};

// look through markdown headers of docs for titles and categories to translate
const docsDir = nodePath.join(CWD, '../', readMetadata.getDocsPath());
let files = glob.sync(`${CWD}/../${readMetadata.getDocsPath()}/**`);
const versionedDocsDir = nodePath.join(CWD, 'versioned_docs');
let files = [
...glob.sync(`${docsDir}/**`),
...glob.sync(`${versionedDocsDir}/**`),
];
files.forEach(file => {
const extension = nodePath.extname(file);
if (extension === '.md' || extension === '.markdown') {
Expand All @@ -75,27 +87,29 @@ function execute() {
return;
}
const metadata = res.metadata;
const id = metadata.localized_id;

translations['localized-strings'][metadata.localized_id] = metadata.title;
translations['localized-strings'].docs[id] = {};
translations['localized-strings'].docs[id].title = metadata.title;

if (metadata.sidebar_label) {
translations['localized-strings'][metadata.sidebar_label] =
translations['localized-strings'].docs[id].sidebar_label =
metadata.sidebar_label;
}
}
});
// look through header links for text to translate
siteConfig.headerLinks.forEach(link => {
if (link.label) {
translations['localized-strings'][link.label] = link.label;
translations['localized-strings'].links[link.label] = link.label;
}
});

// find sidebar category titles to translate
Object.keys(sidebars).forEach(sb => {
const categories = sidebars[sb];
Object.keys(categories).forEach(category => {
translations['localized-strings'][category] = category;
translations['localized-strings'].categories[category] = category;
});
});

Expand All @@ -120,7 +134,7 @@ function execute() {
Object.keys(sidebarContent).forEach(sb => {
const categories = sidebarContent[sb];
Object.keys(categories).forEach(category => {
translations['localized-strings'][category] = category;
translations['localized-strings'].categories[category] = category;
});
});
});
Expand Down Expand Up @@ -170,9 +184,17 @@ function execute() {
translations['pages-strings'],
customTranslations['pages-strings']
);
translations['localized-strings'] = Object.assign(
translations['localized-strings'],
customTranslations['localized-strings']
translations['localized-strings'].links = Object.assign(
Copy link
Contributor

@endiliey endiliey Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Any reason not to simply deepmerge ?

IMHO using Object assign for one level deep and deepmerge for > one level deep is not as declarative as simply deepmerging

translations['localized-strings'] = deepmerge(
  translations['localized-strings'],
  customTranslations['localized-strings']
);

instead of
https://github.com/facebook/Docusaurus/blob/dae9e442a4eb4ed0f8b69254d0f7ae9093dfd34f/lib/write-translations.js#L186-L201

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that one!

translations['localized-strings'].links,
customTranslations['localized-strings'].links
);
translations['localized-strings'].categories = Object.assign(
translations['localized-strings'].categories,
customTranslations['localized-strings'].categories
);
translations['localized-strings'].docs = deepmerge(
translations['localized-strings'].docs,
customTranslations['localized-strings'].docs
);
writeFileAndCreateFolder(
`${CWD}/i18n/en.json`,
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
"commander": "^2.16.0",
"crowdin-cli": "^0.3.0",
"cssnano": "^3.10.0",
"deepmerge": "^2.1.1",
"escape-string-regexp": "^1.0.5",
"express": "^4.15.3",
"feed": "^1.1.0",
Expand Down
4 changes: 4 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1969,6 +1969,10 @@ deep-is@~0.1.3:
version "0.1.3"
resolved "https://registry.yarnpkg.com/deep-is/-/deep-is-0.1.3.tgz#b369d6fb5dbc13eecf524f91b070feedc357cf34"

deepmerge@^2.1.1:
version "2.1.1"
resolved "https://registry.yarnpkg.com/deepmerge/-/deepmerge-2.1.1.tgz#e862b4e45ea0555072bf51e7fd0d9845170ae768"

default-require-extensions@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/default-require-extensions/-/default-require-extensions-2.0.0.tgz#f5f8fbb18a7d6d50b21f641f649ebb522cfe24f7"
Expand Down