Skip to content

Add globalization#202

Merged
0candy merged 1 commit intomasterfrom
initial_glob
Jul 28, 2016
Merged

Add globalization#202
0candy merged 1 commit intomasterfrom
initial_glob

Conversation

@0candy
Copy link
Copy Markdown
Contributor

@0candy 0candy commented Jun 10, 2016 •
edited
Loading

@0candy 0candy added the #review label Jun 10, 2016
@0candy 0candy mentioned this pull request Jun 10, 2016
Closed
63 tasks
@0candy 0candy force-pushed the initial_glob branch 2 times, most recently from 9d5b588 to 7dad264 Compare June 10, 2016 19:05
@0candy 0candy self-assigned this Jun 14, 2016
Setogit
Setogit reviewed Jul 1, 2016
View reviewed changes
Comment thread app/index.js Outdated
this.templates = list.map(function(t) {
return {
name: util.format('%s (%s)', t.name, t.description),
name: g.format('%s (%s)', t.name, t.description),
Copy link
Copy Markdown
Contributor

@Setogit Setogit Jul 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is an interesting case. One can say, "No text involved, here, so no need to translate." Linguist might say, " ( ) in English is used to define supplemental information and the way to specify supplemental information differs from language to language, so, we should globalize and leave it to the translator."

That's a globalization architecture question. For API Connect, we take the former, light-weight approach.

Copy link
Copy Markdown
Member

@bajtos bajtos Jul 13, 2016

Choose a reason for hiding this comment

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

IMO, we need to ensure that at least t.description is localized. This probably means enhancing loopback-workspace with globalization support.

@Setogit
Copy link
Copy Markdown
Contributor

Setogit commented Jul 1, 2016 •
edited
Loading

Line comments added inline. Three top level comments below.

  1. No need to check in intl/MSG.json -- that's for debugging only. We don't check in all the intl//.json but the intl/en/*.json. They are machine translation which can and should be generated when needed.
  2. Where is SetRootDir call?
  3. In case no place holders such as %s, both g.f and g.t work. but g.t is a bit cheaper than g.f

@0candy 0candy force-pushed the initial_glob branch 5 times, most recently from cb1c234 to 7e57a3e Compare July 8, 2016 18:35
@0candy
Copy link
Copy Markdown
Contributor Author

0candy commented Jul 8, 2016 •
edited
Loading

Where is SetRootDir call?

In app/index.js.

@0candy 0candy added this to the #Epic: Globalization milestone Jul 12, 2016
@0candy
Copy link
Copy Markdown
Contributor Author

0candy commented Jul 12, 2016

Failing for node 0.10 windows because of this strongloop/strong-globalize#46

bajtos
bajtos reviewed Jul 13, 2016
View reviewed changes
Comment thread .gitignore Outdated
test/sandbox
test/.pkgcache/
.pkgcache/
intl/MSG.json
Copy link
Copy Markdown
Member

@bajtos bajtos Jul 13, 2016

Choose a reason for hiding this comment

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

Instead of listing all different languages and files, can we afford to git-ignore everything in intl folder?

BTW, keep in mind that npm excludes git-ignored files from the package by default, so we need extra config to bring intl files back when publishing to npmjs.org.

Copy link
Copy Markdown
Contributor Author

@0candy 0candy Jul 21, 2016

Choose a reason for hiding this comment

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

We only need the intl/en directory.
I will change it to

!intl/
intl/*
!intl/en/

@0candy 0candy force-pushed the initial_glob branch 2 times, most recently from 6983067 to 6a29520 Compare July 22, 2016 20:31
@0candy 0candy force-pushed the initial_glob branch 2 times, most recently from 7aabc8d to 295f503 Compare July 27, 2016 14:53
@0candy
Copy link
Copy Markdown
Contributor Author

0candy commented Jul 27, 2016

@jannyHou or @superkhau PTAL. Thanks.

jannyHou
jannyHou reviewed Jul 27, 2016
View reviewed changes
Comment thread app/index.js Outdated

this.option('explorer', {
desc: 'Add Loopback Explorer to the project (true by default)',
desc: g.f('Add Loopback Explorer to the project (true by default)'),
Copy link
Copy Markdown
Contributor

@jannyHou jannyHou Jul 27, 2016

Choose a reason for hiding this comment

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

I personally tend to wrap {{true}} here..

@jannyHou
Copy link
Copy Markdown
Contributor

jannyHou commented Jul 27, 2016 •
edited
Loading

@0candy I don't have confidence whether some terminologies should be wrapped or not, so I comment under some words that IMO will cause users confused if they are accidentally translated to, for example, mandarin.
If they are safe without wrap, please just ignore my picky comment lol.
And other than that, LGTM

@0candy 0candy force-pushed the initial_glob branch 2 times, most recently from 11f9b5f to 25abbd9 Compare July 27, 2016 20:27
@0candy
Copy link
Copy Markdown
Contributor Author

0candy commented Jul 27, 2016

@jannyHou Thanks for reviewing. Fixed.

@jannyHou
Copy link
Copy Markdown
Contributor

jannyHou commented Jul 27, 2016

👍 LGTM

@0candy 0candy merged commit 963523a into master Jul 28, 2016
@0candy 0candy deleted the initial_glob branch July 28, 2016 14:47
@0candy 0candy removed the #review label Jul 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@0candy 0candy

Labels

None yet

Projects

None yet

Milestone

#Epic: Globalization

Development

Successfully merging this pull request may close these issues.

4 participants

@0candy @Setogit @jannyHou @bajtos