-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adding new and missing translations for pt-BR and en-US locales, in particular for the new project visibility feature. #3656
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: develop
Are you sure you want to change the base?
Conversation
@raclim, notice that once again I had to search inside the codebase for english texts, extracting them to proper locale entries, as shown below: ![]() This could be avoided if new features are only accepted by maintainers when they FULLY support i18n, as already discussed. |
@raclim, is the target date for the 2.0 version really August of 2026 (and not 2025)? Notice the ![]() |
newVisibility === 'Public' | ||
? t('Visibility.Public.Label').toLowerCase() | ||
: t('Visibility.Private.Label').toLowerCase() | ||
}); |
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 could also be written as:
const visibilityToastText = t('Visibility.Changed', {
projectName,
newVisibility: t(`Visibility.${newVisibility}.Label`).toLowerCase()
});
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raclim, I have updated the code to a much more flexible switch
statement, as shown below:
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 union
type containing all visibility options ('Public', 'Private', 'Protected' etc) and then use a technique known as "exhaustiveness checking" (https://www.typescriptlang.org/docs/handbook/2/narrowing.html#exhaustiveness-checking) in order to make sure all options were properly considered in the switch
statement (you will get a compilation error if any options are missing, which is really cool). Something like:
// 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 exhaustiveCheck
}
}
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.
Follows another good example where this technique could be used:
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 entries 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 switch
statement, ideally using a union type and the "exhaustiveness checking" technique, as the following code shows:
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 is catch it at compile time:

"ChatOnDiscord":"Chat On Discord", | ||
"PostOnTheForum":"Post on the Forum" | ||
"ChatOnDiscord": "Chat On Discord", | ||
"PostOnTheForum": "Post on the Forum" |
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.
Notice the missing spaces before the Object values, which were re-introduced automatically by Prettier.
"LibraryVersion": "Versão do p5.js", | ||
"LibraryVersionInfo": "Há uma [nova versão 2.0](https://github.com/processing/p5.js/releases/) do p5.js disponível! Ela se tornará padrão em agosto de 2026, então aproveite este tempo para testá-la e relatar bugs. Está interessado em transitar esboços de 1.x para 2.0? Confira os [recursos de compatibilidade e transição.](https://github.com/processing/p5.js-compatibility)", | ||
"LibraryVersionInfo": "Há uma [nova versão 2.0](https://github.com/processing/p5.js/releases/) do p5.js disponível! Ela se tornará padrão em agosto de 2026, então aproveite este tempo para testá-la e relatar bugs. Está interessado em migrar esboços de 1.x para 2.0? Confira os [recursos de compatibilidade e migração.](https://github.com/processing/p5.js-compatibility)", | ||
"CustomVersionTitle": "Gerenciando suas próprias bibliotecas? Legal!", |
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.
I have used the proper pt-BR verb here ("migrar" instead of "transitar").
…t from Portuguese of Portugal
ko: '한국어', | ||
'pt-BR': 'Português', | ||
'pt-BR': 'Português do Brasil', | ||
sv: 'Svenska', |
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.
@raclim, should I go ahead and create an issue for this? Or will you do it yourself? |
I just opened an issue (and attached it to this PR) to start a discussion on how we’d like to implement this process. At the moment, we're planning to merge in one more feature in the near future (#3594). Once that's in, we’ll probably be holding off on merging in features for a while. Since we're aiming to merge it in soon and the translation changes in that PR are relatively small, I think afterwards we can dedicate time to figure out a sustainable translation flow for new features!
Yes, it's quite a while from now, but the target date is 2026! |
@raclim, we have 2 distinct subjects here:
|
@raclim, please let me know if you have any other doubts about this PR, which is important in order to fix |
Related to #3666
Adding new and missing translations for pt-BR and en-US locales, in particular for the new project visibility feature.
Changes:
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.