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

Add localization #49

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

maxbeier
Copy link

@maxbeier maxbeier commented Jan 22, 2022

This PR adds basic translation capabilities.

Description of change

It adds the react-intl lib to support language specific strings. It also adds ESLint rules to nudge people to replace static strings with dynamic ones. Locale strings have support for basic HTML elements (b, i, ul, p, br).

The following components are introduced:

  • LocaleProvider which wraps the app and provides the correct messages for the current locale with EN as a fallback per message
  • Translate which receives a key and optional some values and renders the appropriate text

It also adds a useLocale hook to get the current locale and messages, all available locales, and a function to change the active locale.

Furthermore it adds a shell script that can be used to e.g. check PRs for divergency in locale keys.

Related Issues

How do I test this?

  • load page → should either use DE or EN depending on your OS settings
  • change locale on the footer → language should have changed
  • reload page → selected language should still be correct

Is there something controversial in your PR?

  • I removed all defaultMessage props from the Translate component to force devs to add new keys to at least one locale file. This promotes locale files to be the source of truth and enabled us to diff against them to find missing translations. On the other hand, it also makes the code harder to read and understand as one has to manually look up all texts from the locale files.
  • Unused locale keys are hard to detect and remove.

Github Workflow

As one cannot add GitHub workflows in a PR, maybe someone with access wants to adds this to .github/workflows/diff-locale-keys.yml

name: diff-locale-keys

on:
  push:
    paths:
      - 'src/locale/*.json'

jobs:
  generate-icons:
    runs-on: ubuntu-latest

    steps:
      - name: checkout files
        uses: actions/checkout@v2
        with:
          ref: ${{ github.head_ref }}

      - name: generate diff
        id: diff
        run: |
          OUTPUT=$(bash scripts/diff_locale_keys.sh)
          OUTPUT="${OUTPUT//'%'/'%25'}"
          OUTPUT="${OUTPUT//$'\n'/'%0A'}"
          OUTPUT="${OUTPUT//$'\r'/'%0D'}"
          echo "::set-output name=result::$OUTPUT"

      - name: get number of PR
        uses: jwalton/gh-find-current-pr@v1
        id: finder

      - name: comment on PR
        if: ${{ steps.diff.outputs.result != '' }}
        uses: marocchino/sticky-pull-request-comment@v2
        with:
          number: ${{ steps.finder.outputs.pr }}
          header: diff
          message: |
            Thanks a lot for your PR! The following keys are not available in all locale files – maybe you could help to translate them?
            ```
            ${{ steps.diff.outputs.result }}
            ```

@maxbeier maxbeier marked this pull request as draft January 22, 2022 15:32
@pajowu
Copy link
Member

pajowu commented Jan 22, 2022

😍

@maxbeier maxbeier marked this pull request as ready for review January 25, 2022 13:26
@maxbeier
Copy link
Author

@pajowu PR is ready for review ✌️


# get all locale files and put them into an array
IFS=$'\n'
LOCALE_FILES=($(ls src/locale/*.json))
Copy link

@AntonLydike AntonLydike Jan 31, 2022

Choose a reason for hiding this comment

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

I would shy away from using ls src/locale/*.json (it's weird with some file names, see this)

I would suggest to use LOCALE_FILES=(src/locale/*.json) instead, as it handles a lot of edge cases much better (and is much better to read, since you can skip the IFS parts around it)

</p>

<div style={{ flexGrow: 1 }} />
<BigBackButton content={'Zurück'} />
<BigBackButton content="Zurück" />

Choose a reason for hiding this comment

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

I think you missed a spot here?

'N+S': 'Nucleoprotein + Spike',
};

const getReadableAntigen = (antigen: string) => antigenMap[antigen] || antigen;

Choose a reason for hiding this comment

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

I think you missed a translate call here?

Copy link
Author

Choose a reason for hiding this comment

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

would consider it a technical term and not translate it

initialValue?: JSONValue<T>
): [typeof state, typeof setState] {
const [state, setState] = useState<T>(
() => safeJsonParse(localStorage.getItem(key)) || initialValue

Choose a reason for hiding this comment

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

You define defaultValue as an argument of the function safeJsonParse, but ignore it here and instead use || initialValue, which will handle edge cases such as falsy JSON values (such as null or []) differently.

I am not saying this is the wrong way, it's just something that should probably be cleaned up before merging.

import { useNavigate, useParams } from 'react-router';
import { get_test, TestData } from './testdata';

export type JSONValue<T = any> =

Choose a reason for hiding this comment

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

Is this really necessary? It just evaluates to any in every place it is used in this project, or am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

ok, removed it

# Conflicts:
#	.eslintrc.json
#	package.json
#	src/components/Result/TestFound.tsx
#	src/components/Result/TestFoundButLegalThreats.tsx
#	src/components/Scanner/BarcodeScannerComponent.tsx
#	src/components/Scanner/HowToOverlay.tsx
#	src/routes/About.tsx
#	src/routes/MoreInformation.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants