-
Notifications
You must be signed in to change notification settings - Fork 92
DT-824 - set up i18n #1362
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.
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
DT-824 - set up i18n #1362
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
fix bug with replace and count both specified
const SRC_DIR = path.resolve('./src/lib/i18n/locales'); | ||
const DEST_DIR = path.resolve('./static/i18n/locales'); | ||
|
||
const logAndExit = (msg: string, logError: (msg: string) => void) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We copied this from the Temporal CLI download script right? I wonder if we should break this out and put it… somewhere? (Not for this PR, of course.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's slightly different because this script can be run via pnpm
or as a vite plugin during the build/serve commands. Vite plugins' Context have an error
function: https://rollupjs.org/plugin-development/#this-error so I'm passing it through to here instead of using console.error
when running as a vite plugin. This way any vite cleanup stuff can still happen normally instead of immediately exiting the process.
} | ||
}; | ||
|
||
const relativeTo = (path: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit much, but I'm into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, I think I wrote it before I knew it was going to be a one liner.
scripts/generate-locales.ts
Outdated
); | ||
} | ||
|
||
const sourceFileSymbol = checker.getSymbolAtLocation(sourceFile!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the !
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Telling the TypeScript compiler that we do have a defined sourceFile
here, because we are checking for its truthiness in the lines above. https://github.com/temporalio/ui/pull/1362/files#diff-c113f7c7730c2f6cf15e9c61f2bb2be52ea8921825a0dd8f1aac12eed4f75a22R34-R39
I could refactor the code to avoid these assertions, but then I think it becomes slightly less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I can just add some return
's so that the compiler is happy, will do that.
scripts/generate-locales.ts
Outdated
logAndExit(`No default export in source: ${source}`, logError); | ||
} | ||
|
||
const jsObject: Record<string, string> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate the name of this object, but naming stuff is hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do better 😂
if (grandchild.kind === ts.SyntaxKind.PropertyAssignment) { | ||
let key = ''; | ||
let value = ''; | ||
const [keyNode, valueNode] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about this. But if you're going to pop it into an array just to destructure it and that takes four lines, maybe you just do two variable assignments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this does seem unnecessary now that I think about it
* If more suffixes are needed, i.e. `_few`, add them here. | ||
*/ | ||
type WithoutPluralSufix<T> = T extends | ||
| `${infer P}_zero` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*slow clap*
remove i18n on workflows-with-new-search for now
Description & motivation 💭
This PR lays a foundation for internationalization of the UI using
i18next
.i18next
,i18next-http-backend
, andi18next-browser-languagedetector
dependenciesi18n.init()
script tosrc/routes/+layout.ts
and set thei18n
store on$page.data
<Translate />
componentScreenshots (if applicable) 📸
N/a
Design Considerations 🎨
N/a
Testing 🧪
How was this tested 👻
Steps for others to test: 🚶🏽♂️🚶🏽♀️
Checklists
N/a
Draft Checklist
Merge Checklist
Issue(s) closed
Docs
N/a
Any docs updates needed?