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: add language selection on settings page #842

Conversation

oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented Jan 22, 2024

fixes #830
closes #875

@oleonardolima
Copy link
Contributor Author

@futurepaul I'm curious about how I should override the i18n context, is it already being done on another part of the code?

@oleonardolima
Copy link
Contributor Author

@futurepaul I'm curious about how I should override the i18n context, is it already being done on another part of the code?

As we discussed, it would need a new language field and function update, right ?

Something like this:

async saveFiat(fiat: Currency) {
localStorage.setItem("fiat_currency", JSON.stringify(fiat));
const price = await actions.fetchPrice(fiat);
setState({
price: price,
fiat: fiat
});
},

@futurepaul
Copy link
Collaborator

Something like this:

yeah that looks right to me. you set it in the state, and then you reference that value from the global state to pass to the config during i18npovider (https://github.com/MutinyWallet/mutiny-web/blob/master/src/components/I18nProvider.tsx)

it looks like i18n has its cachuserlanguage thing but probably don't need to bother with it would rather just have it stored in megastore https://github.com/i18next/i18next-browser-languageDetector

@oleonardolima
Copy link
Contributor Author

Something like this:

yeah that looks right to me. you set it in the state, and then you reference that value from the global state to pass to the config during i18npovider (https://github.com/MutinyWallet/mutiny-web/blob/master/src/components/I18nProvider.tsx)

it looks like i18n has its cachuserlanguage thing but probably don't need to bother with it would rather just have it stored in megastore https://github.com/i18next/i18next-browser-languageDetector

nice! thx

@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch 2 times, most recently from ba17300 to 1bf2d77 Compare January 25, 2024 16:48
@oleonardolima
Copy link
Contributor Author

....

it looks like i18n has its cachuserlanguage thing but probably don't need to bother with it would rather just have it stored in megastore https://github.com/i18next/i18next-browser-languageDetector

yep, I've tried using the cache and detection options, but doesn't to work 🤔, I'll try getting it directly from megaStore then lol

@benalleng
Copy link
Collaborator

benalleng commented Jan 31, 2024

I see the issue here! you need to modify the detection in the i18n config as such

        detection: {
            order: ["querystring", "localStorage", "navigator", "htmlTag"],
            lookupQuerystring: "lang",
            lookupLocalStorage: "i18nextLng",
            caches: ["localStorage"], // not sure if this specific key is necessry
        },

This will allow the localStorage item to be found

@oleonardolima
Copy link
Contributor Author

I see the issue here! you need to modify the detection in the i18n config as such

        detection: {
            order: ["querystring", "localStorage", "navigator", "htmlTag"],
            lookupQuerystring: "lang",
            lookupLocalStorage: "i18nextLng",
            caches: ["localStorage"], // not sure if this specific key is necessry
        },

This will allow the localStorage item to be found

Nice! I've tried that, but the problem remained I think mainly because it wasn't being refreshed 🤔

@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch 2 times, most recently from 2b57676 to d24d5de Compare February 9, 2024 14:07
Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

Really close! great work, just some small things and then make sure to run pnpm pre-commit to clean up any prettier formatting

@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch from d24d5de to c51f59b Compare February 10, 2024 13:47
@oleonardolima
Copy link
Contributor Author

Really close! great work, just some small things and then make sure to run pnpm pre-commit to clean up any prettier formatting

@benalleng I was talking with @benthecarman about adding some non-blocking step on CI for showing that some translations are missing, wdyt ?

@benalleng
Copy link
Collaborator

Definitely! I think we should probably make a new PR for that CI check though

@oleonardolima
Copy link
Contributor Author

Definitely! I think we should probably make a new PR for that CI check though

I'll add some playwright tests on this PR as well, and then we are good to go :)

@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch from c51f59b to b74eb18 Compare February 10, 2024 14:57
@oleonardolima oleonardolima changed the title wip(feat): add language selection on settings page feat: add language selection on settings page Feb 10, 2024
@oleonardolima oleonardolima marked this pull request as ready for review February 10, 2024 14:58
@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch from b74eb18 to 1048318 Compare February 10, 2024 15:27
Copy link
Collaborator

@benalleng benalleng Feb 10, 2024

Choose a reason for hiding this comment

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

from the element chat we came to some consensus that we might want to change the key names for thie Language interface as it might be a little confusing with the value being the "label" in the ui and the label being the "value" sent to i18next. @futurepaul suggested we replace label with shortName

Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

Made another pass after I had a chance to look at #875 and the value <=> label stuff was most of the problem with the select options being weird but I did catch one small change in addition to that

@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch from 1048318 to 9aa390b Compare February 11, 2024 15:23
@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch 2 times, most recently from 1534c08 to 605a19a Compare February 11, 2024 15:29
Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

Sorry to butt heads on this! But switching back and forth with the value and findLanguageByValue() is still causing problems.

If you start in another language it still always displays english as the default

@oleonardolima oleonardolima force-pushed the feat/add-language-selection-on-settings-menu branch from 605a19a to 111b042 Compare February 11, 2024 16:35
@oleonardolima
Copy link
Contributor Author

Sorry to butt heads on this! But switching back and forth with the value and findLanguageByValue() is still causing problems.

If you start in another language it still always displays english as the default

No worries, I think I covered everything now

Copy link
Collaborator

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

works for me

Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

tACK great work on this!

@benthecarman benthecarman merged commit 075c066 into MutinyWallet:master Feb 11, 2024
3 of 4 checks passed
@oleonardolima oleonardolima deleted the feat/add-language-selection-on-settings-menu branch February 11, 2024 18:55
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.

feature: add language options on settings page
4 participants