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

Improve i18n utility, add English fallbacks #186

Merged
merged 8 commits into from Aug 29, 2023
Merged

Conversation

JSKitty
Copy link
Member

@JSKitty JSKitty commented Aug 25, 2023

Abstract

This PR adds a well-needed cleanup to the way devs interface with the i18n system.

  • We now have a tr() utility function, for in-line translations of strings with variables.
  • The i18n system now uses the English file as a fallback if a key is missing.
  • Translation switching is now a per-key process, rather than entire file.

This means a few small changes to how we previously used it, for ex:

Translating variables in-line is now super easy, i.e:

// i18n key `hello` is "Hello {user}!"
// Which becomes "Hello Bob!"
const user = 'Bob';
const str = tr(translation.hello, [{ user: user }]);

Alerts no longer have the additional variables param, devs are now expected to use tr() in-line to translate alerts on-the-fly.

Before

// With variable
createAlert('success', ALERTS.SUCCESSFUL_TX, [{ txid: txid }], 5000);
// Without variable
createAlert('success', ALERTS.SUCCESSFUL_TX, [], 5000);

After

// With variable
createAlert('success', tr(ALERTS.SUCCESSFUL_TX, [{ txid: txid }]), 5000);
// Without variable
createAlert('success', tr(ALERTS.SUCCESSFUL_TX), 5000);

Previously, dynamic JS-selected translations would return '' or undefined to the user in the GUI, if the i18n keys were missing or unset for their language; with the English fallback, this is no longer the case, and users will never be presented with undefined or empty text now.


Testing

To test this PR, it's suggested to attempt these user flows, or variations of these:

  • Change your language (multiple times across a few testing sessions and wallets).
  • Import or Create a wallet.
  • Test various parts of the wallet, ensuring you see no suspicious Notifications or Text which is either empty, or says undefined. (Send TXs, try causing errors, change settings, create a proposal on Testnet, etc).

If any errors are found, the PR works unexpectedly, or you have viable suggestions to improve the UX or functionality of the PR, let me know!


@JSKitty JSKitty added Awaiting Review This PR and/or issue is awaiting reviews before continuing. Review Reward: 20 PIV Reviewers of this Pull Request will receive a 20 PIV reward labels Aug 25, 2023
@JSKitty JSKitty requested review from Liquid369, BreadJS and a team August 25, 2023 10:14
@JSKitty JSKitty self-assigned this Aug 25, 2023
@JSKitty JSKitty added Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request labels Aug 25, 2023
Liquid369
Liquid369 previously approved these changes Aug 28, 2023
Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK 16aebc8
Continually getting better for the i18n system translations 🔥

Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK cc87b58
Attacked this a few times, it looks much better now and well continue to overhaul the i18n!

@JSKitty JSKitty merged commit 6bee33f into master Aug 29, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review This PR and/or issue is awaiting reviews before continuing. Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request Review Reward: 20 PIV Reviewers of this Pull Request will receive a 20 PIV reward
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants