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

feat-i18n-api-messages #3156

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

feat-i18n-api-messages #3156

wants to merge 15 commits into from

Conversation

mind-ar
Copy link
Collaborator

@mind-ar mind-ar commented Apr 9, 2022

This PR implements i18n for api messages and Closes #2834
This first commit implements a concept of work, it needs validation from maintainers to be able to continue

@authelia
Copy link

authelia bot commented Apr 9, 2022

Thanks for choosing to contribute @mind-ar. We lint all PR's with golangci-lint and eslint, I may add a review to your PR with some suggestions.

You are free to apply the changes if you're comfortable, alternatively you are welcome to ask a team member for advice.

Artifacts

These changes once approved by a team member will be published for testing on Buildkite, DockerHub and GitHub Container Registry.

Docker Container

  • docker pull authelia/authelia:PR3156
  • docker pull ghcr.io/authelia/authelia:PR3156

@mind-ar mind-ar marked this pull request as draft April 9, 2022 17:29
web/src/services/Client.ts Outdated Show resolved Hide resolved
web/src/services/Client.ts Outdated Show resolved Hide resolved
@mind-ar mind-ar marked this pull request as ready for review April 13, 2022 01:16
@james-d-elliott
Copy link
Member

Just upon initial inspection, the backend namespace is unused. You need to load it when calling the useTranslation hook where appropriate (i.e. when the messages from the backend are used). The syntax is like useTranslation(["backend"]) or useTranslation(["portal", "backend"]) depending on if you're using one or both.

@mind-ar
Copy link
Collaborator Author

mind-ar commented Apr 13, 2022

Hi @james-d-elliott sorry for not clarifying.
This PR is mostly for adding missing translations.

regarding your question: the namespaces are loaded in web/src/i18n/index.ts.
To use the 'backend' namespace for specific translation, you must use translate("some text", {ns: "backend"})
Unfortunately there is no case currently in use to test this, as we are masking the backend response with a different text, for example:

        try {
            const res = await postFirstFactor(username, password, rememberMe, redirectionURL, requestMethod);
            props.onAuthenticationSuccess(res ? res.redirect : undefined);
        } catch (err) {
            console.error(err);
            createErrorNotification(translate("Incorrect username or password"));
            props.onAuthenticationFailure();
            setPassword("");
            passwordRef.current.focus();
        }

To use the backend namespace we would to use somethink like this

        try {
            const res = await postFirstFactor(username, password, rememberMe, redirectionURL, requestMethod);
            props.onAuthenticationSuccess(res ? res.redirect : undefined);
        } catch (err) {
            console.error(err);
            createErrorNotification(translate(err.message, {ns: "backend"}));
            props.onAuthenticationFailure();
            setPassword("");
            passwordRef.current.focus();
        }

So, this PR only gives the "readyness" for backend translations, because that requires that we display the backend's message to the user (maybe not desired)

Let me know what you think

@mind-ar
Copy link
Collaborator Author

mind-ar commented Aug 6, 2022

Hello, I think this PR could be discarded as it does not provide any visible improvement

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.

sugestion: i18n for snackbar notificaitons
3 participants