Fix bug wrong metadata on translated docs
#709
Conversation
|
Deploy preview for docusaurus-preview ready! Built with commit 45f16c8 |
lib/server/readMetadata.js
Outdated
| let regexSubFolder = new RegExp( | ||
| '/' + escapeStringRegexp(getDocsPath()) + '/(.*)/.*/' | ||
| ); | ||
| let language = _language; |
There was a problem hiding this comment.
why don't we delete this proposed line 128 and replace _language at proposed line 125 to be language
Save one line of code ? 😃
There was a problem hiding this comment.
This 3 lines is moved to the next line.
There was a problem hiding this comment.
Because the code is not needed in this situations if language is given.
There was a problem hiding this comment.
I mean
function processMetadata(file, language) {instead of
function processMetadata(file, _language) {
let language = _language;There was a problem hiding this comment.
I think no-param-reassign is good code style.
https://eslint.org/docs/rules/no-param-reassign
There was a problem hiding this comment.
Of course if you want to change it, I will change it. :)
There was a problem hiding this comment.
I actually had that doubt as well. I'll leave it to @yangshun & @JoelMarcey to decide ☺
lib/server/readMetadata.js
Outdated
| let regexSubFolder = new RegExp( | ||
| '/' + escapeStringRegexp(getDocsPath()) + '/(.*)/.*/' | ||
| ); | ||
| let language = _language; |
There was a problem hiding this comment.
Because the code is not needed in this situations if language is given.
lib/server/readMetadata.js
Outdated
| let regexSubFolder = new RegExp( | ||
| '/' + escapeStringRegexp(getDocsPath()) + '/(.*)/.*/' | ||
| ); | ||
| let language = _language; |
There was a problem hiding this comment.
I think no-param-reassign is good code style.
https://eslint.org/docs/rules/no-param-reassign
lib/server/readMetadata.js
Outdated
| let regexSubFolder = new RegExp( | ||
| '/' + escapeStringRegexp(getDocsPath()) + '/(.*)/.*/' | ||
| ); | ||
| let language = _language; |
There was a problem hiding this comment.
Of course if you want to change it, I will change it. :)
| @@ -271,7 +273,7 @@ function generateMetadataDocs() { | |||
| const extension = path.extname(file); | |||
|
|
|||
| if (extension === '.md' || extension === '.markdown') { | |||
| const res = processMetadata(file); | |||
| const res = processMetadata(file, language); | |||
There was a problem hiding this comment.
Don't you want to pass language in to the other calls to processMetadata?
e.g., on line 219 in this file:
const res = processMetadata(file);
There was a problem hiding this comment.
We only need to specify the language case, if we use translated_docs. In general, we do not need to specify, because we use getDocsPath. It seems to increase unnecessary processing.
There was a problem hiding this comment.
Hi @gimdongwoo - In general though, I don't like leaving a parameter missing to a function call. It can cause confusion in reading the code. Even if you don't pass language -- which I still think is ok - can you pass something, even if a ''?
|
I know a lot more bugs related to this project's multilingual, and I was fixed it already. However, there are many cases where we need to change the structure a lot, so I want to send PR in gradually. I do not think nobody actually using this project as a multilingual environment. Even |
😮 What are some of these bugs? |
|
Https://reasonml.github.io use multi language support. I think its best for @gimdongwoo to let us know on these bugs 😊 Anyway, i actually found out about this bug too when working on subdirectories, athough it will be fixed on #705. |
|
We are running our docs in 3 languages and hope to support 2 more. So I don't believe its true that no one is running multilingual. http://loomx.io/developers |
|
This largely fixes our issues in the sidebar with Japanese and Chinese language seeping into the English docs |
|
Oh I saw that @mattkanwisher. Chinese meta in your english docs |
|
Closed in favor of #705 |
Motivation
There is a bug where metadata files are generated incorrectly when using multilingual features.
When
processMetadatafunction is called intranslated_docs, the logic to determine the multilingual language has bug.We already knew the language so We did not have to do it twice.
[Incorrect -
core/metadata.js]The language is reversed (en - ko).
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
using multilingual features.
[Correct -
core/metadata.js]Related PRs
e273dfc#diff-a6ea8ce8e7c294d50b747139bc956138