Adding new and missing translations for pt-BR and en-US locales, in particular for the new project visibility feature. #3656
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -413,7 +413,7 @@ export function deleteProject(id) { | |
| }); | ||
| }; | ||
| } | ||
| export function changeVisibility(projectId, projectName, visibility, t) { | ||
| return (dispatch, getState) => { | ||
| const state = getState(); | ||
|
|
||
|
|
@@ -443,11 +443,25 @@ export function changeVisibility(projectId, projectName, visibility) { | |
| name: response.data.name | ||
| }); | ||
|
|
||
| let visibilityLabel; | ||
|
|
||
| switch (newVisibility) { | ||
| case 'Public': | ||
| visibilityLabel = t('Visibility.Public.Label'); | ||
| break; | ||
| case 'Private': | ||
| visibilityLabel = t('Visibility.Private.Label'); | ||
| break; | ||
| default: | ||
| visibilityLabel = newVisibility; | ||
| } | ||
|
|
||
| const visibilityToastText = t('Visibility.Changed', { | ||
| projectName, | ||
| newVisibility: visibilityLabel.toLowerCase() | ||
| }); | ||
|
There was a problem hiding this comment. This could also be written as: but as a general rule of thumb introducing locale entries in the source code containing interpolated strings SHOULD BE AVOIDED, since this makes finding unused translations later much harder. There was a problem hiding this comment. I could see this being adjusted at some point, since we plan to add more visibility options in the future. Maybe there's a way to utilize some kind of dynamic key here? There was a problem hiding this comment. @raclim, I have updated the code to a much more flexible let visibilityLabel;
switch (newVisibility) {
case 'Public':
visibilityLabel = t('Visibility.Public.Label');
break;
case 'Private':
visibilityLabel = t('Visibility.Private.Label');
break;
default:
visibilityLabel = newVisibility;
}
const visibilityToastText = t('Visibility.Changed', {
projectName,
newVisibility: visibilityLabel.toLowerCase()
});It now becomes much easier to add more visibility options in the future, e.g. a 'Protected' one: switch (newVisibility) {
case 'Public':
visibilityLabel = t('Visibility.Public.Label');
break;
case 'Private':
visibilityLabel = t('Visibility.Private.Label');
break;
case 'Protected':
visibilityLabel = t('Visibility.Protected.Label');
break;
default:
visibilityLabel = newVisibility;
}When using TypeScript (hopefully soon), my suggestion is to futurely create a TS // In some other module.
type VisibilityOptions = 'Public' | 'Private' | 'Protected'
switch (newVisibility) { // `newVisibility` will be of type `VisibilityOptions`
case 'Public':
visibilityLabel = t('Visibility.Public.Label');
break;
case 'Private':
visibilityLabel = t('Visibility.Private.Label');
break;
case 'Protected':
visibilityLabel = t('Visibility.Protected.Label');
break;
default: {
const exhaustiveCheck: never = newVisibility
throw new Error(`Unhandled case: ${exhaustiveCheck}`);
}
}There was a problem hiding this comment. Follows another good example where this technique could be used which is, according to my analysis, the only case of a "dynamic" locale key in your entire codebase: export const SkipLink = ({ targetId, text }: SkipLinkProps) => {
// ...
return (
<a
href={`#${targetId}`}
className={linkClasses}
onFocus={handleFocus}
onBlur={handleBlur}
>
{t(`SkipLink.${text}`)}
</a>
);
};Because, like I mentioned before, interpolating text in order to produce locale keys makes finding them later in the source code an almost impossible task, be it manually or using some automated tool/script. My recommendation would be to replace the above code with a type SkipLinkOptions = 'PlaySketch'; // More could be added later, e.g. 'PlaySketch' | 'StopSketch'
interface SkipLinkProps {
targetId: string;
text: SkipLinkOptions;
}
export const SkipLink = ({ targetId, text }: SkipLinkProps) => {
const [focus, setFocus] = useState(false);
const { t } = useTranslation();
const handleFocus = () => {
setFocus(true);
};
const handleBlur = () => {
setFocus(false);
};
const linkClasses = classNames('skip_link', { focus });
let translatedText: string;
switch (text) {
case 'PlaySketch':
translatedText = t('SkipLink.PlaySketch');
break;
default: {
const exhaustiveCheck: never = text;
throw new Error(`Unhandled case: ${exhaustiveCheck}`);
}
}
return (
<a
href={`#${targetId}`}
className={linkClasses}
onFocus={handleFocus}
onBlur={handleBlur}
>
{translatedText}
</a>
);
};So, it any new options are added later, but no translations are provided, TS will catch it at compile time: 
There was a problem hiding this comment. PS: the above There was a problem hiding this comment. Thanks so much for the detailed explanation, I'm still relatively new to TypeScript myself, so this was really helpful! I agree that using union types with exhaustiveness checking would be a great improvement with i18n. I think it could make sense as a follow-up PR if that sounds good! |
||
|
|
||
| dispatch(setToastText(visibilityToastText)); | ||
| dispatch(showToast(2000)); | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Renaming menu entry to "Português do Brasil", in order to distinguish it from Portuguese of Portugal.