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

Initial work on i18n mechanics #109

Merged
merged 4 commits into from
Nov 5, 2021
Merged

Initial work on i18n mechanics #109

merged 4 commits into from
Nov 5, 2021

Conversation

jleach
Copy link
Contributor

@jleach jleach commented Nov 3, 2021

Summary of Changes

Implement a basic i18n strategy using react-i18next as per the related issue.

Related Issues

Fixes #110

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this).
  • Updated LICENSE-3RD-PARTY.md for any added dependencies or vendored components.
  • Run prettier: npm run style-format
  • Updated documentation for changed code and new or modified features.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

PR template adapted from the Python attrs project.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Is there a particular reason you went with localized-strings?

It hasn't been updated in 2 years and isn't one of the major libraries out there (AFAIK).

We use i18next and react-i18next in our wallet, maybe something to look into? Also fine with using this library, just curious for the reasoning

@jleach
Copy link
Contributor Author

jleach commented Nov 3, 2021

@TimoGlastra No reason other that it was a fairly simple implementation. I'll look into react-i18next and see how it can be adapted to this project.

@NeilSMyers
Copy link
Contributor

would this take the language their phone is set to automatically?

@jleach
Copy link
Contributor Author

jleach commented Nov 3, 2021

@NeilSMyers That's the idea. It will find the best available matching translation based on what the phone is set for.

@NeilSMyers
Copy link
Contributor

@jleach what do you think of having a way to manually change the language? Idk if this use case is too small to worry about but if someone has their phone on English but wants the app in French for clarity's sake

@jleach
Copy link
Contributor Author

jleach commented Nov 3, 2021

@NeilSMyers I think its a great feature. Well placed in the "settings" once we get to implementing more functionality there. I've made a note about this and will check with the team if we're going to keep it in our board or use Issues (probably issues).

@TimoGlastra Switched to use react-i18next.

@jleach jleach self-assigned this Nov 4, 2021
@jleach jleach changed the title Initial work on i8n mechanics Initial work on i18n mechanics Nov 4, 2021
@TimoGlastra
Copy link
Contributor

Awesome!

I'll try to share some typescript goodies later today that can make the 't()' method fully typed. This way it will error if you put in a string that doesn't have a translation

App/i18n/en/index.ts Outdated Show resolved Hide resolved
@TimoGlastra
Copy link
Contributor

TimoGlastra commented Nov 4, 2021

For adding typescript integration with react-i18next: Add a type Translation that is based on the en translation. Other languages can now set this as the type, meaning that if en adds a property we will get an error if it is not added for other languages.

export type Translation = typeof en

Other language:

const translation: Translation = {

}

Then create a types/react-i18next.d.ts file and add the following to it:

import { Translation } from '../src/translations' # update with your path

import 'react-i18next'

declare module 'react-i18next' {
  interface CustomTypeOptions {
    resources: {
      translation: Translation
    }
  }
}

The t() method from useTranslation() is now fully typed

Signed-off-by: Jason C. Leach <jason.leach@fullboar.ca>
Signed-off-by: Jason C. Leach <jason.leach@fullboar.ca>
Signed-off-by: Jason C. Leach <jason.leach@fullboar.ca>
@jleach
Copy link
Contributor Author

jleach commented Nov 4, 2021

@TimoGlastra Updated to include type support.

Signed-off-by: Jason C. Leach <jason.leach@fullboar.ca>
@jleach jleach marked this pull request as ready for review November 4, 2021 23:32
@jleach jleach requested a review from a team as a code owner November 4, 2021 23:32
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Nice!

@jleach jleach merged commit 06788da into openwallet-foundation:main Nov 5, 2021
@jleach jleach deleted the feature/i8n branch November 5, 2021 15:17
jcdrouin21 pushed a commit to MCN-ING/aries-mobile-agent-react-native that referenced this pull request May 16, 2023
…c-test-integration

remove unnessesary bs4 import in gapi auth code interface
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.

Add support for multiple languages (l18n)
3 participants