Skip to content

Conversation

@VeraZab
Copy link
Contributor

@VeraZab VeraZab commented Dec 13, 2017

No description provided.

@VeraZab VeraZab force-pushed the list-all-localized-strings branch 2 times, most recently from 2042850 to 915fb1b Compare December 14, 2017 05:36
@VeraZab VeraZab changed the title Start Add script to extract locale strings Dec 14, 2017
@VeraZab VeraZab force-pushed the list-all-localized-strings branch from 915fb1b to 9038707 Compare December 14, 2017 16:18
@VeraZab
Copy link
Contributor Author

VeraZab commented Dec 14, 2017

@bpostlethwaite ready for a look
cc @alexcjohnson

.join('\n');
const pathToTranslationKeys = path.join(__dirname, 'translationKeys.txt');
fs.writeFile(pathToTranslationKeys, strings);
console.log('ok find_locale_strings');
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

or maybe not? Dunno

});

if (!hasTranslation) {
console.error('Found no translations.');
Copy link
Member

Choose a reason for hiding this comment

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

logError?

glob(srcGlob, (err, files) => {
if (err) {
EXIT_CODE = 1;
console.log(err);
Copy link
Member

Choose a reason for hiding this comment

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

console.error ?

Copy link
Member

Choose a reason for hiding this comment

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

or this can simply be if (err) throw(err)

Copy link
Member

Choose a reason for hiding this comment

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

that will automatically use exitcode 1 and console.error anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see there is an error handler below that catches these things.

@bpostlethwaite bpostlethwaite self-requested a review December 14, 2017 16:25
Copy link
Member

@bpostlethwaite bpostlethwaite left a comment

Choose a reason for hiding this comment

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

Do we want to commit the translations? They are not set in stone yet and can be produced from the script so are more of a build artifact than an asset. Not sure though.

@VeraZab
Copy link
Contributor Author

VeraZab commented Dec 14, 2017

yeah, I can remove the translations, I believe @alexcjohnson committed them in the plotly.js repo, but it's true, it's not really necessary

if (!dict[strNodeValue]) {
dict[strNodeValue] =
filePartialPath + ':' + strNode.loc.start.line;
maxLen = Math.max(maxLen, strNodeValue.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

might want to put a limit on this... 80? so a few of the longest strings would not have the line-number comment lined up with the rest (just give them at least 2 spaces), but the majority of them would and it'd be closer and easier to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of like that they're all aligned, even if there are some long stings, I find it's more consistent for my eyes :) and makes me notice the super long strings, i.e. may be a sign that they'd need to be shortened...

@alexcjohnson
Copy link
Collaborator

Do we want to commit the translations?

I did for plotly.js, the rationale being that

  • we take the opportunity right now to get them as good as we can - and use the fact that we've collected them all in one file to improve consistency.
  • if something changes, we can more easily determine exactly what's new/changed, to help out the translators.

Continuing will convert your note to LaTeX-style text. // /components/widgets/text_editors/MultiFormatTextEditor.js:116
Continuing will remove your expression. // /components/widgets/text_editors/MultiFormatTextEditor.js:126
Custom // /DefaultEditor.js:179
Custom colors // /components/widgets/ColorPicker.js:52
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we've generally capitalized all words of titles, so Custom Colors? Default Colors? Less sure about Enter Link URL...

Text // /DefaultEditor.js:364
Text Attributes // /DefaultEditor.js:82
The anchor point determines which side of the annotation's positioning coordinates refer to. // /DefaultEditor.js:256
The positioning inputs are relative to the anchor points on the text box // /DefaultEditor.js:389
Copy link
Collaborator

Choose a reason for hiding this comment

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

.

Copy link
Member

Choose a reason for hiding this comment

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

This is a nice way to correct all the text in a program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, agreed, making corrections now :)

@VeraZab
Copy link
Contributor Author

VeraZab commented Dec 14, 2017

ok, made the changes @bpostlethwaite @alexcjohnson mergeable?

@alexcjohnson
Copy link
Collaborator

💃

@bpostlethwaite
Copy link
Member

nice work! 💃

@VeraZab VeraZab merged commit d68c5f3 into master Dec 14, 2017
@VeraZab VeraZab deleted the list-all-localized-strings branch December 14, 2017 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants