8000 DT-824 - set up i18n by rossedfort · Pull Request #1362 · temporalio/ui · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 9 commits into from
May 17, 2023
Merged

DT-824 - set up i18n #1362

merged 9 commits into from
May 17, 2023

Conversation

rossedfort
Copy link
Contributor
@rossedfort rossedfort commented May 12, 2023

Description & motivation 💭

This PR lays a foundation for internationalization of the UI using i18next.

  • add i18next, i18next-http-backend, and i18next-browser-languagedetector dependencies
  • add the i18n.init() script to src/routes/+layout.ts and set the i18n store on $page.data
  • add a <Translate /> component
  • add a script to generate JSON from TypeScript locale files so that we can have strongly typed keys

Screenshots (if applicable) 📸

N/a

Design Considerations 🎨

N/a

Testing 🧪

How was this tested 👻

  • Manual testing
  • E2E tests added
  • Unit tests added

Steps for others to test: 🚶🏽‍♂️🚶🏽‍♀️

Checklists

N/a

Draft Checklist

Merge Checklist

Issue(s) closed

Docs

N/a

Any docs updates needed?

@vercel
Copy link
vercel bot commented May 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
holocene ⬜️ Ignored (Inspect) May 17, 2023 4:26pm

@rossedfort rossedfort marked this pull request as ready for review May 15, 2023 16:19
@rossedfort rossedfort requested review from stevekinney, Alex-Tideman and a team as code owners May 15, 2023 16:19
@rossedfort rossedfort requested review from laurakwhit and removed request for Alex-Tideman May 15, 2023 17:55
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) => {
Copy link
Contributor

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.)

Copy link
Contributor Author

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) => {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

);
}

const sourceFileSymbol = checker.getSymbolAtLocation(sourceFile!);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the !?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

logAndExit(`No default export in source: ${source}`, logError);
}

const jsObject: Record<string, string> = {};
Copy link
Contributor

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.

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 can do better 😂

if (grandchild.kind === ts.SyntaxKind.PropertyAssignment) {
let key = '';
let value = '';
const [keyNode, valueNode] = [
Copy link
Contributor

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?

Copy link
Contributor Author

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`
Copy link
Contributor

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
@rossedfort rossedfort merged commit bc79544 into main May 17, 2023
@rossedfort rossedfort deleted the DT-824-i18n-setup branch May 17, 2023 20:26
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.

2 participants
0