feat: add i18n in login page by zhaozhiming · Pull Request #5947 · coder/code-server

zhaozhiming · 2023-01-05T01:36:47Z

jsjoeio

Great work so far! Mostly a couple minor things and questions.

jsjoeio · 2023-01-05T15:33:13Z

    ],
-    "downlevelIteration": true
+    "downlevelIteration": true,
+    "resolveJsonModule": true


TIL!

https://www.typescriptlang.org/tsconfig#resolveJsonModule

Yes, this is for importing the locale JSON files in typescript.

jsjoeio · 2023-01-05T15:33:29Z

      expect(htmlContent).toContain(`Welcome to ${appName}`)
    })
+
+    it("should return correct welcome text when lng is set to non-English", async () => {


Thank you for adding a test 👏🏼

jsjoeio · 2023-01-05T15:34:14Z

      <div class="card-box">
        <div class="header">
          <h1 class="main">{{WELCOME_TEXT}}</h1>
-          <div class="sub">Please log in below. {{PASSWORD_MSG}}</div>


Part of me thinks we should keep the " " here between {{I18N_LOGIN_BELOW}}{{PASSWORD_MSG}} since I could see someone forgetting to add it to the locales file.

jsjoeio · 2023-01-05T15:34:43Z

      content="style-src 'self'; script-src 'self' 'unsafe-inline'; manifest-src 'self'; img-src 'self' data:; font-src 'self' data:;"
    />
-    <title>{{APP_NAME}} login</title>
+    <title>{{APP_NAME}} {{I18N_LOGIN_TITLE}}</title>


Re: keeping space...we do it here, so I think we should keep this pattern.

Also should the app name be part of the login title? Not sure if in some languages the app name might come after the word for login.

ok. I will put the app name into the whole title.

jsjoeio · 2023-01-05T15:35:20Z

  verbose?: boolean
  "app-name"?: string
  "welcome-text"?: string
+  "lng"?: string


nit: I'd vote for lang over lng. I think that's more common? Thoughts?

cc @code-asher

I would actually prefer we use the existing --locale flag since that is what Code uses.

Good point!

I’m a little worried about whether it affects other places when using the existing --locale flag?

Hmm...the only place I know is the display language but there may be others.

Yup --locale is only used to set Code's display language.

So for example if someone wanted to change the language of code-server and Code all on the command line they would have to do something like:

code-server --lng zh-cn --locale zh-cn

So it seems unfortunate to have to write the same thing twice and it would not be clear to the user why there are two separate flags. In the future this means we can also support changing code-server's display language from the UI which would be cool.

ok. That's right. I will change to use the --locale flag.

jsjoeio · 2023-01-05T15:36:02Z

+      Language show on login page, more infomations to read up on
+      https://en.wikipedia.org/wiki/IETF_language_tag.


Suggested change

Language show on login page, more infomations to read up on

https://en.wikipedia.org/wiki/IETF_language_tag.

Language to show on the login page, more info see

https://en.wikipedia.org/wiki/IETF_language_tag

I had put the description to the --locale flag.

jsjoeio · 2023-01-05T15:39:50Z

  const appName = req.args["app-name"] || "code-server"
-  const welcomeText = req.args["welcome-text"] || `Welcome to ${appName}`
-  let passwordMsg = `Check the config file at ${humanPath(os.homedir(), req.args.config)} for the password.`
+  const welcomeText = req.args["welcome-text"] || (i18n.t("WELCOME", { app: appName }) as string)


Is this as string necessary? I would think t returns a string

This is the thing I'm struggling with.
Because the t returns string or null( t return type DefaultTFuncReturn).
Although I added returnNull to the i18n configuration, this config item makes the t never return null.
But the TS compiler doesn't seem to know about this, so I need to add as string.
I'd also like to know if there is a better way to avoid adding as string.

We can always look in a followup PR. I don't want to hold you back so it's fine as is for now. Thank you for trying that!

code-asher · 2023-01-05T20:11:18Z

@@ -0,0 +1,13 @@
+{


Would you be willing to add yourself as a code owner for this file in .github/CODEOWNERS in case we add new strings in the future?

Yes. It's my pleasure.

codecov · 2023-01-05T22:59:48Z

Codecov Report

Merging #5947 (9f50607) into main (8377bd2) will increase coverage by 0.13%.
The diff coverage is 86.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5947      +/-   ##
==========================================
+ Coverage   71.62%   71.75%   +0.13%     
==========================================
  Files          30       31       +1     
  Lines        1688     1696       +8     
  Branches      371      372       +1     
==========================================
+ Hits         1209     1217       +8     
  Misses        401      401              
  Partials       78       78

Impacted Files	Coverage Δ
src/node/cli.ts	`91.76% <ø> (ø)`
src/node/routes/login.ts	`87.93% <80.00%> (+0.65%)`	⬆️
src/node/i18n/index.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d40a974...9f50607. Read the comment docs.

jsjoeio

Looking really good! I think last thing is using lang instead of lng.

jsjoeio · 2023-01-06T16:29:03Z


 docs/install.md @GNUxeava
+
+src/node/i18n/locales/zh-cn.json @zhaozhiming


Note to self: may have to give write access...

Related: https://github.com/orgs/community/discussions/23042

I don't know much about .github/CODEOWNERS. Is there anything else I need to do?

No, you're good here!

jsjoeio

Nice work! Thanks again for doing this!

zhaozhiming · 2023-01-13T01:36:58Z

@code-asher Is there anything else I need to do in this PR?

code-asher

Thank you!!

zhaozhiming requested a review from a team as a code owner January 5, 2023 01:36

zhaozhiming force-pushed the feat-i18n-login branch from 5f11fac to 70b9780 Compare January 5, 2023 01:52

jsjoeio reviewed Jan 5, 2023

View reviewed changes

code-asher reviewed Jan 5, 2023

View reviewed changes

zhaozhiming added 2 commits January 6, 2023 10:31

feat: add i18n in login page

ade3da3

fix: add word space and put the app name into the title

b53a57a

zhaozhiming force-pushed the feat-i18n-login branch from 70b9780 to b53a57a Compare January 6, 2023 06:02

zhaozhiming added 4 commits January 6, 2023 14:07

fix: remove duplicate replace title

9aa26f4

fix: prettier format code

d66eed1

fix: fix typescript check warning

e94f368

fix: add zh-cn locale file code owner

eed3122

jsjoeio reviewed Jan 6, 2023

View reviewed changes

fix: use existing flag locale to the login page

8eabb16

jsjoeio approved these changes Jan 9, 2023

View reviewed changes

code-asher approved these changes Jan 13, 2023

View reviewed changes

Merge branch 'main' into feat-i18n-login

9f50607

jsjoeio enabled auto-merge (squash) January 13, 2023 17:23

jsjoeio merged commit 7c2aa8c into coder:main Jan 13, 2023

		Language show on login page, more infomations to read up on
		https://en.wikipedia.org/wiki/IETF_language_tag.


		docs/install.md @GNUxeava

		src/node/i18n/locales/zh-cn.json @zhaozhiming No newline at end of file

Conversation

zhaozhiming commented Jan 5, 2023

Uh oh!

jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

zhaozhiming commented Jan 13, 2023

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

code-asher Jan 6, 2023 •

edited

Loading

code-asher Jan 5, 2023 •

edited

Loading

codecov Bot commented Jan 5, 2023 •

edited

Loading