Skip to content
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

chore(react-i18next): change usage of custom useTranslation to react-i18next #6443

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Feb 2, 2024

Issue

Description

Updates out translation code to make use of i18n interpolation instead of sprintf-js. This should help with performance, developer experience and bundle size.

Todo:

  • Remove usage of custom useTranslation hook
  • Fix type errors
  • Fix tests
  • Update translation files
  • Remove old custom code

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

web/src/App.tsx Fixed Show fixed Hide fixed
Comment on lines +18 to +29
setupNodeEvents(on, _config) {
on('before:browser:launch', (browser, launchOptions) => {
if (browser.family === 'chromium' && browser.name !== 'electron') {
launchOptions.preferences.default.intl = {
accept_languages: 'en-US,en',
selected_languages: 'en-US,en',
};

return launchOptions;
}
});
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just so the local setup uses the same as the CI everywhere. Was quite annoying when it picked Swedish instead of English all the time.

@@ -15,7 +14,6 @@ export default function MapOptionSelector({
children,
isMobile,
}: MapOptionSelectorProps) {
const { __ } = useTranslation();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we consider any variable that begins with _ to be a placeholder/allowed unused we didn't catch these unused hooks before.

There are a few more that I removed as well.

…-vsprintf-and-use-i18n-interpolation-instead
@VIKTORVAV99
Copy link
Member Author

Sorry for the massive PR guys, should be pretty straight forward to review though.

Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fingers hurt from scrolling, but it all looks great and on the preview app everything looks as expected! Thanks for taking the time to do this one, it's cleaning up a lot of tech debt 🙏 💚

web/src/App.tsx Show resolved Hide resolved
Comment on lines -163 to 164
export function getBadgeText(chartData: AreaGraphElement[]) {
const { __ } = useTranslation();

export function getBadgeText(chartData: AreaGraphElement[], t: TFunction) {
const allEstimated = chartData.every(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice with this change so we don't need the hook in here :)

@VIKTORVAV99 VIKTORVAV99 added the techdebt Unpleasantness that does (or may in future) affect development label Feb 22, 2024
@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Feb 23, 2024

@madsnedergaard should we wait for a review from Tony or Silke as well or merge this now? I would like to merge this as soon as possible since it changes 88 files and merge conflicts are only a matter of time...

@madsnedergaard
Copy link
Member

@madsnedergaard should we wait for a review from Tony or Silke as well or merge this now? I would like to merge this as soon as possible since it changes 88 files and merge conflicts are only a matter of time...

I think it's fine to merge it, I reviewed it thoroughly (famous last words 😉)

@VIKTORVAV99 VIKTORVAV99 merged commit f23d939 into master Feb 23, 2024
21 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/ele-3692-remove-vsprintf-and-use-i18n-interpolation-instead branch February 23, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨 performance 🏎 techdebt Unpleasantness that does (or may in future) affect development translations 🗣
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants