I18n setup #189
I18n setup #189
Conversation
There was a problem hiding this comment.
@eddiekeller you've done it again!
This is looking great! Some small nits that I'd like your opinion on, but the side-effects usage is the one change that we should consider before merging.
packages/react/src/components/Authenticator/FederatedSignIn/FederatedSignIn.tsx
Outdated
Show resolved
Hide resolved
| @@ -1,7 +1,8 @@ | |||
| import { useAuth } from '../../../hooks'; | |||
| import { I18n } from '@aws-amplify/core'; | |||
| import { getAliasInfoFromContext } from '@aws-amplify/ui-core'; | |||
There was a problem hiding this comment.
Somebody did Organize Imports ;)
There was a problem hiding this comment.
Unorganized imports give me gas :)
examples/next/pages/ui/components/authenticator/sign-in-federated/index.page.tsx
Outdated
Show resolved
Hide resolved
eddiekeller
force-pushed
the
i18n-setup
branch
from
9e84bb2
to
985b440
Compare
There was a problem hiding this comment.
All lgtm 🚢 Just got some general nits, but nothing worth blocking over. Thanks @eddiekeller!
| @@ -0,0 +1,312 @@ | |||
| /* | |||
| * Copyright 2017-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Should we change it to 2017-2021, or do we even need this header?
There was a problem hiding this comment.
copy and pasted from the old copyright in the JS repo. not sure if we need it. None of the other files in this new repo have it.
| 'Enter your username': 'Geben Sie Ihren Benutzernamen ein', | ||
| 'Enter your password': 'Geben Sie Ihr Passwort ein', |
There was a problem hiding this comment.
While working on this PR, did you find any missing translations that should be added here?
e.g. we would need to add Enter your email and Enter your password eventually.
There was a problem hiding this comment.
I haven't looked at all the screens yet - just sign in for now. But you're right! I'm sure we will need to add some / modify some eventually.
| }, | ||
|
|
||
| es: { | ||
| 'Sign in to your account': 'Iniciar sesíon', |
There was a problem hiding this comment.
NIT: I'm wondering if we should have a type that enumerates these translation keys to ensure that language translations are in parity.
Perhaps something like:
type TranslationEntry =
| 'Sign in to your account'
| 'Resend Code'
// ...
type BilingualDict = Record<TranslationEntry, string>;
type SupportedLanguages = 'de' | 'fr' | 'es' | 'zh' | 'ja';
type Dict = Record<SupportedLanguages, BilingualDict>There was a problem hiding this comment.
Bringing this up because I see that "Signing In" only exist in German.
There was a problem hiding this comment.
We can add in typing...but I wonder if "forcing" parity between the languages would work. We are going to rely on the open source community for translation support, and I'm picturing a scenario when a contributor will want to add a new string, or only a couple strings of translations. In that case, forcing parity between all languages might almost act like a blocker. Maybe I'm thinking about it wrong.
There was a problem hiding this comment.
Ah, that's a fair point. I agree, let's keep it as is now. Maybe we can do Partial<> just so that we have vscode autocomplete, but this is not an issue now and I don't want it to be a blocker.
| <Input name="password" required type="password" /> | ||
| <Box> | ||
| <Text>Forgot your password?</Text>{' '} | ||
| <Text>{I18n.get('Forgot your password? ')}</Text> |
There was a problem hiding this comment.
NIT: imo space ' ' shouldn't be added in translation and instead be handled by UI.
There was a problem hiding this comment.
I think translations actually make more sense. Example - Japanese doesn't use spaces. Forcing spaces in the UI would be breaking certain languages. If the intent of the space isn't an actual linguistic space but whitespace between elements, then we should be handling that via CSS instead.
There was a problem hiding this comment.
Yeah, but I think with that component the space was used to put spacing between "Forgot your password" and the link if I remember correctly. Which I think should be handled through css or {{' '}}
Issue #, if available:
n/a
Description of changes:
adding initial setup for i18n support. only added translated text to the
SignInpage for now until we decide that this is a good path forward. screenshots showing japanese and french -By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.