-
-
Notifications
You must be signed in to change notification settings - Fork 614
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
Include built-in UI translation for regional docs #475
Conversation
🦋 Changeset detectedLatest commit: 30015cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
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.
This looks great @Yan-Thomas! Would you be up for writing a couple of tests for the cases you mentioned?
You could expand on translations.test.ts
and translations-with-user-config.test.ts
.
Let me know if you have questions about this. In the test with user config, the fake i18n dictionaries are created here:
starlight/packages/starlight/__tests__/basics/translations-with-user-config.test.ts
Lines 5 to 9 in 6a7692a
vi.mock('astro:content', async () => | |
(await import('../test-utils')).mockedAstroContent({ | |
i18n: [['en', { 'page.editLink': 'Modify this doc!' }]], | |
}) | |
); |
Re: docs, I don’t know how much are needed. In general, I don’t think there’s new API here, just smoothing over use of regional specifiers.
Co-authored-by: Chris Swithinbank <[email protected]>
Sure, will take care of making some tests!
Yeah, I feel like probably just a note on the internationalization page would suffice! I am leaving for today but will add that as part of the PR tomorrow! |
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.
Adding a note so I don’t forget — this needs a changeset. I’d suggest this is a patch as it fixes usage that is arguably expected.
Co-Authored-By: Chris Swithinbank <[email protected]>
Note that I haven't added tests to ensure the built-in regional variant gets precedence over the base language one, it seems we don't even have support for them yet, since the // import statements can't have dashes
// and renaming to something like `zhTw`
// won't match with the user-provided lang.
import zh-TW from './zh-TW.json';
const { parse } = builtinI18nSchema();
export default Object.fromEntries(
Object.entries({ cs, en, es, de, ja, pt, fr, it, nl, da, tr, ar, nb, zh }).map(([key, dict]) => [
key,
parse(dict),
])
); |
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.
Thanks @Yan-Thomas — this looks good! Suggesting two docs tweaks and then should be good to go 🙌
Oh we could still support it! We just couldn’t rely on the shorthand object notation: import en from './en.json';
import zhTW from './zh-TW.json';
const { parse } = builtinI18nSchema();
export default Object.fromEntries(
Object.entries({ en, 'zh-TW': zhTW }).map(([key, dict]) => [
key,
parse(dict),
])
); |
Co-authored-by: Chris Swithinbank <[email protected]>
Co-authored-by: Chris Swithinbank <[email protected]>
Co-Authored-By: Chris Swithinbank <[email protected]>
Oh nice! Tested locally and worked perfectly, we're good to ship then! 🚀 |
Co-authored-by: Chris Swithinbank <[email protected]>
What kind of changes does this PR include?
Description
This PR makes it possible for regional languages, e.g.
pt-BR
(Brazilian Portuguese) to use the built-in UI translation for the main language (e.gpt
).I've intentionally made that this would only work for the built-in translations, so you still can create an
i18n/pt-BR.json
file and override the built-in translation in case you want.I've made a fairly simple case-insensitive function looking for the expected BCP-47 tag, we could throw an error if it isn't a valid string or make some more complex type checking for it, but I went simple to not overengineer for now.
TO-DO: docs! (let's see if we're going to change the API or something before)
Testing
The feature was tested with a copy branch of #469, with the following steps:
getting-started.mdx
translation forpt-BR
, the UI elements should be translated with the built-in UI i18n ✅i18n/pt-BR.json
, the UI elements should ignore the built-in UI and use the user-provided file instead ✅