-
Notifications
You must be signed in to change notification settings - Fork 17
feat(i18n): add per-instance language selection mechanism #1442
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,7 @@ export class SetupService { | |
| // older $BrandingConfig will silently drop newer branding fields on read. | ||
| const branding = $BrandingConfig.nullable().safeParse(savedOptions?.branding ?? null); | ||
| return { | ||
| activeLanguages: savedOptions?.activeLanguages?.length ? savedOptions.activeLanguages : ['en', 'fr'], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the first of four copies of Export
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thomasbeaudry I agree |
||
| branding: branding.success ? branding.data : null, | ||
| isDemo: Boolean(savedOptions?.isDemo), | ||
| isExperimentalFeaturesEnabled: Boolean(savedOptions?.isExperimentalFeaturesEnabled), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,32 @@ | ||
| /* eslint-disable @typescript-eslint/consistent-type-definitions */ | ||
| /* eslint-disable @typescript-eslint/no-namespace */ | ||
|
|
||
| import { i18n } from '@douglasneuroinformatics/libui/i18n'; | ||
| import type { Language } from '@douglasneuroinformatics/libui/i18n'; | ||
|
|
||
| declare module '@douglasneuroinformatics/libui/i18n' { | ||
| export namespace UserConfig { | ||
| export interface LanguageOptions { | ||
| en: true; | ||
| es: true; | ||
| fr: true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const VALID_LANGUAGES = ['en', 'es', 'fr']; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Third hardcoded language list in this PR (after Two behavioural notes:
Minor: the two
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thomasbeaudry I agree
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not change the eslint-disable though |
||
|
|
||
| function detectLanguage(): Language { | ||
| if (typeof window !== 'undefined') { | ||
| const param = new URLSearchParams(window.location.search).get('lang'); | ||
| if (param && VALID_LANGUAGES.includes(param)) { | ||
| return param as Language; | ||
| } | ||
| } | ||
| return 'en'; | ||
| } | ||
|
|
||
| i18n.init({ | ||
| defaultLanguage: detectLanguage(), | ||
| translations: {} | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,11 +124,11 @@ const LogoSection = ({ alignment, branding, instanceName, preview }: LogoSection | |
| type ResourcesSectionProps = { | ||
| boldResourceLinks: boolean; | ||
| fontSize: null | number | undefined; | ||
| lang: 'en' | 'fr'; | ||
| lang: string; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unacceptable. We do not use magic strings in this codebase. |
||
| links: ResourceLink[]; | ||
| preview: boolean; | ||
| tc: (slateClass: string) => null | string; | ||
| tl: (obj: { en: string; fr: string }) => string; | ||
| tl: (obj: { [key: string]: string }) => string; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use mapped type |
||
| }; | ||
|
|
||
| const ResourcesSection = ({ boldResourceLinks, fontSize, lang, links, preview, tc, tl }: ResourcesSectionProps) => ( | ||
|
|
@@ -144,8 +144,13 @@ const ResourcesSection = ({ boldResourceLinks, fontSize, lang, links, preview, t | |
| style={fontStyle(fontSize, preview)} | ||
| > | ||
| {links.map((link, index) => { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const linkLabel = link.label?.[lang]?.trim() || link.label?.en?.trim() || link.label?.fr?.trim() || ''; | ||
| /* eslint-disable @typescript-eslint/prefer-nullish-coalescing -- blank strings must fall back, so `||` not `??` */ | ||
| const linkLabel = | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is very messy. Add a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be typed properly with generics |
||
| (link.label as undefined | { [key: string]: string })?.[lang]?.trim() || | ||
| link.label?.en?.trim() || | ||
| link.label?.fr?.trim() || | ||
| ''; | ||
| /* eslint-enable @typescript-eslint/prefer-nullish-coalescing */ | ||
| if (!linkLabel) return null; | ||
| return ( | ||
| <a | ||
|
|
@@ -224,15 +229,17 @@ export const LoginBrandingPanel = ({ | |
| const { resolvedLanguage } = useTranslation(); | ||
| const lang = langOverride ?? resolvedLanguage; | ||
|
|
||
| const tl = (obj: { en: string; fr: string }): string => obj[lang]; | ||
| const tl = (obj: { [key: string]: string }): string => obj[lang] ?? obj.en ?? ''; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mapped types |
||
|
|
||
| const derived = useMemo(() => { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const instanceName = branding?.instanceName?.[lang]?.trim() || DEFAULT_INSTANCE_NAME; | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const instanceTagline = branding?.instanceTagline?.[lang]?.trim() || null; | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const instanceDetails = branding?.instanceDetails?.[lang]?.trim() || null; | ||
| /* eslint-disable @typescript-eslint/prefer-nullish-coalescing -- blank strings must fall back, so `||` not `??` */ | ||
| const instanceName = | ||
| (branding?.instanceName as undefined | { [key: string]: string })?.[lang]?.trim() || DEFAULT_INSTANCE_NAME; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These casts (four in this file, plus The root cause is that const $LocalizedString = z.partialRecord($Language, z.string().nullish());That also makes Note #1439 adds a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
| const instanceTagline = | ||
| (branding?.instanceTagline as undefined | { [key: string]: string })?.[lang]?.trim() || null; | ||
| const instanceDetails = | ||
| (branding?.instanceDetails as undefined | { [key: string]: string })?.[lang]?.trim() || null; | ||
| /* eslint-enable @typescript-eslint/prefer-nullish-coalescing */ | ||
| const panelTextColor = branding?.panelTextColor ?? null; | ||
| return { | ||
| boldDetails: branding?.boldDetails === true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,9 @@ import { MenuIcon, StopCircle } from 'lucide-react'; | |
|
|
||
| import { useIsDesktop } from '@/hooks/useIsDesktop'; | ||
| import { useNavItems } from '@/hooks/useNavItems'; | ||
| import { useSetupStateQuery } from '@/hooks/useSetupStateQuery'; | ||
| import { useAppStore } from '@/store'; | ||
| import { ALL_LANGUAGES } from '@/utils/languages'; | ||
|
|
||
| import { GroupSwitcher, useIsGroupSwitcherVisible } from '../GroupSwitcher'; | ||
| import { NavButton } from '../NavButton'; | ||
|
|
@@ -23,6 +25,12 @@ export const Navbar = () => { | |
| const { t } = useTranslation('layout'); | ||
| const navigate = useNavigate(); | ||
| const endSession = useAppStore((store) => store.endSession); | ||
| const setupStateQuery = useSetupStateQuery(); | ||
|
|
||
| const activeLanguages = setupStateQuery.data.activeLanguages ?? ['en', 'fr']; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see comments above |
||
| const languageOptions = Object.fromEntries( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be a shared hook |
||
| Object.entries(ALL_LANGUAGES).filter(([key]) => activeLanguages.includes(key)) | ||
| ); | ||
|
|
||
| // This is to prevent ugly styling when resizing the viewport | ||
| const isDesktop = useIsDesktop(); | ||
|
|
@@ -124,13 +132,9 @@ export const Navbar = () => { | |
| <div className="flex w-full justify-between gap-2 md:justify-end"> | ||
| <UserDropup /> | ||
| <div className="flex gap-2"> | ||
| <LanguageToggle | ||
| options={{ | ||
| en: 'English', | ||
| fr: 'Français' | ||
| }} | ||
| variant="outline" | ||
| /> | ||
| {Object.keys(languageOptions).length > 1 && ( | ||
| <LanguageToggle options={languageOptions} variant="outline" /> | ||
| )} | ||
| <ThemeToggle variant="outline" /> | ||
| </div> | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import React from 'react'; | ||
|
|
||
| import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; | ||
| import { CheckIcon, Loader2Icon } from 'lucide-react'; | ||
|
|
||
| export const SaveStatus = ({ state }: { state: 'idle' | 'saved' | 'saving' }) => { | ||
| const { t } = useTranslation(); | ||
| if (state === 'idle') { | ||
| return null; | ||
| } | ||
| return ( | ||
| <div className="fixed bottom-4 right-4 z-50 flex items-center gap-1.5 rounded-full border border-slate-200/70 bg-white/95 px-3 py-1.5 text-xs font-medium shadow-md backdrop-blur dark:border-slate-700/70 dark:bg-slate-800/95"> | ||
| {state === 'saving' ? ( | ||
| <React.Fragment> | ||
| <Loader2Icon className="text-muted-foreground h-3.5 w-3.5 animate-spin" /> | ||
| <span className="text-muted-foreground">{t({ en: 'Saving…', es: 'Guardando…', fr: 'Enregistrement…' })}</span> | ||
| </React.Fragment> | ||
| ) : ( | ||
| <React.Fragment> | ||
| <CheckIcon className="h-3.5 w-3.5 text-green-600" /> | ||
| <span> | ||
| {t({ en: 'All changes saved', es: 'Todos los cambios guardados', fr: 'Modifications enregistrées' })} | ||
| </span> | ||
| </React.Fragment> | ||
| )} | ||
| </div> | ||
| ); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,9 @@ import { StopCircle } from 'lucide-react'; | |
| import { AnimatePresence, motion } from 'motion/react'; | ||
|
|
||
| import { useNavItems } from '@/hooks/useNavItems'; | ||
| import { useSetupStateQuery } from '@/hooks/useSetupStateQuery'; | ||
| import { useAppStore } from '@/store'; | ||
| import { ALL_LANGUAGES } from '@/utils/languages'; | ||
|
|
||
| import { GroupSwitcher, useIsGroupSwitcherVisible } from '../GroupSwitcher'; | ||
| import { NavButton } from '../NavButton'; | ||
|
|
@@ -27,9 +29,15 @@ export const Sidebar = () => { | |
| const endSession = useAppStore((store) => store.endSession); | ||
| const location = useLocation(); | ||
| const navigate = useNavigate(); | ||
| const setupStateQuery = useSetupStateQuery(); | ||
|
|
||
| const { t } = useTranslation(); | ||
|
|
||
| const activeLanguages = setupStateQuery.data.activeLanguages ?? ['en', 'fr']; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comments above and shared hook please |
||
| const languageOptions = Object.fromEntries( | ||
| Object.entries(ALL_LANGUAGES).filter(([key]) => activeLanguages.includes(key)) | ||
| ); | ||
|
|
||
| return ( | ||
| <div | ||
| className="flex h-screen w-[19rem] flex-col bg-slate-900 px-3 py-2 text-slate-100 shadow-lg dark:border-r dark:border-slate-700" | ||
|
|
@@ -137,16 +145,15 @@ export const Sidebar = () => { | |
| <div className="flex items-center justify-between py-2"> | ||
| <UserDropup /> | ||
| <div className="flex h-full items-center gap-2"> | ||
| <LanguageToggle | ||
| contentClassName="bg-slate-800 border-slate-700 text-slate-300" | ||
| itemClassName="bg-slate-800 hover:bg-slate-700 focus:bg-slate-700 focus:text-slate-100" | ||
| options={{ | ||
| en: 'English', | ||
| fr: 'Français' | ||
| }} | ||
| triggerClassName="hover:bg-slate-800 hover:text-slate-300 focus-visible:ring-0" | ||
| variant="ghost" | ||
| /> | ||
| {Object.keys(languageOptions).length > 1 && ( | ||
| <LanguageToggle | ||
| contentClassName="bg-slate-800 border-slate-700 text-slate-300" | ||
| itemClassName="bg-slate-800 hover:bg-slate-700 focus:bg-slate-700 focus:text-slate-100" | ||
| options={languageOptions} | ||
| triggerClassName="hover:bg-slate-800 hover:text-slate-300 focus-visible:ring-0" | ||
| variant="ghost" | ||
| /> | ||
| )} | ||
| <ThemeToggle className="hover:bg-slate-800 hover:text-slate-300" variant="ghost" /> | ||
| </div> | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -212,7 +212,9 @@ export const StartSessionForm = ({ | |
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: | ||
| currentGroup.settings.idValidationRegexErrorMessage?.[resolvedLanguage] ?? | ||
| (currentGroup.settings.idValidationRegexErrorMessage as undefined | { [key: string]: string })?.[ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no casting at call sites |
||
| resolvedLanguage | ||
| ] ?? | ||
| t({ | ||
| en: `Must match regular expression: ${regex.source}`, | ||
| fr: `Doit correspondre à l'expression régulière : ${regex.source}` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,14 @@ export function useInstrument(id: null | string) { | |
| const { bundle, id } = instrumentBundleQuery.data; | ||
| interpreter | ||
| .interpret(bundle, { id, validate: import.meta.env.DEV }) | ||
| .then((instrument) => setInstrument(translateInstrument(instrument, resolvedLanguage))) | ||
| .then((instrument) => | ||
| setInstrument( | ||
| translateInstrument( | ||
| instrument, | ||
| resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
One named helper says what it means and gives you a single place to change the policy: /** Instruments are authored in en/fr only; other UI languages fall back to English. */
export const toInstrumentLanguage = (language: Language): 'en' | 'fr' =>
language === 'fr' ? 'fr' : 'en';The policy itself is also worth surfacing: a user who has selected Español gets instruments silently rendered in English with no indication that a translation is missing. That's defensible, but right now it's an implicit consequence of five scattered ternaries rather than a stated decision.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. Translate instrument should handle this already downstream. I think it already does |
||
| ) | ||
| ) | ||
| ) | ||
| .catch((err) => { | ||
| console.error(err); | ||
| setInstrument(null); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,8 @@ export function useInstrumentInfoQuery<TKind extends InstrumentKind>({ | |
| params: { ...params, groupId: currentGroupId } | ||
| }); | ||
| const infos = await $InstrumentInfo.array().parseAsync(response.data); | ||
| return infos.map((instrument) => translateInstrumentInfo(instrument, resolvedLanguage ?? 'en')); | ||
| const instrumentLang = resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en'; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be handled downstream |
||
| return infos.map((instrument) => translateInstrumentInfo(instrument, instrumentLang)); | ||
| }, | ||
| queryKey: [ | ||
| 'instrument-info', | ||
|
|
||
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.
A required scalar list added to a model with existing documents. #1441 adds its
SetupStatefield asInt?; this one has neither?(not permitted on lists) nor@default([]).Prisma's MongoDB connector should return
[]for a list field absent from a stored document, so this probably works — but "probably" is doing real work in a field that gates the language toggle for the whole instance.@default([])costs nothing and removes the question for every instance created before this deploys.Also worth noting
initAppcreatesSetupStatewithout this field, so the very first document an instance writes exercises exactly that path.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.
@thomasbeaudry I agree, add the default