Implemented Language Selector # 35 #49

2025-03-03T16:38:45-08:00

Vutukuri15 commented

2025-03-03 16:38:45 -08:00

(Migrated from github.com)

This PR implements multi-language support in the poker chip distribution app. Users can now select their preferred language from the app settings, and all text elements (buttons, labels, instructions) update accordingly.

MantashaNoyela (Migrated from github.com) reviewed 2025-03-03 16:38:45 -08:00

djwesty (Migrated from github.com) approved these changes 2025-03-03 22:36:15 -08:00

djwesty (Migrated from github.com) left a comment

Nice work on this one. Good job keeping things organized and modular, and the small fixes to tests to make things more consistent.

Do we need an update for package.json for the new i18n related packages? I do not see a diff for that file.
I took a quick look at the react-i18n docs to follow up on our discussion about how to actually use the translations based on the two approaches

My interpretation of the docs is that the way like this

import i18next from './i18n'
i18next.t('my.key')

is geared towards accessing translations from outside a React component.

The other way, using the hook is intended for use from inside functional components (like all of our app translations).

const { t } = useTranslation();

I am not suggesting you change this (unless you want to) because consistency is most important and we have that. However, keep this in mind for future projects.

I also looked at the docs for expo-localization. This seems like a nice way to try to get the phones default language, as well as its currency. If you enjoyed working on these features and wanted to add more functionality it could be interesting to have our app try to automatically configure these settings rather than use the hard-coded defaults ('$' and 'en'). I don't see this feature as something our app requires for MVP, it's just an idea if you want more things to work on. This would also be slightly tricky to test because you would need to change your phones region and language back and forth several times

Nice work on this one. Good job keeping things organized and modular, and the small fixes to tests to make things more consistent. 1) Do we need an update for `package.json` for the new `i18n` related packages? I do not see a diff for that file. 2) I took a quick look at the [react-i18n docs](https://docs.expo.dev/versions/unversioned/sdk/localization/) to follow up on our discussion about how to actually use the translations based on the two approaches My interpretation of the docs is that the way like this ``` import i18next from './i18n' i18next.t('my.key') ``` is geared towards accessing translations from _outside_ a React component. The other way, using the hook is intended for use from inside functional components (like all of our app translations). ``` const { t } = useTranslation(); ``` I am not suggesting you change this (unless you want to) because consistency is most important and we have that. However, keep this in mind for future projects. 3) I also looked at the docs for [expo-localization](https://docs.expo.dev/versions/latest/sdk/localization/). This seems like a nice way to try to get the phones default language, as well as its currency. If you enjoyed working on these features and wanted to add more functionality it could be interesting to have our app try to automatically configure these settings rather than use the hard-coded defaults ('$' and 'en'). I don't see this feature as something our app requires for MVP, it's just an idea if you want more things to work on. This would also be slightly tricky to test because you would need to change your phones region and language back and forth several times

i18n/es.json

djwesty (Migrated from github.com) commented

2025-03-03 22:07:51 -08:00

I think these should be lowercase, for the sake of consistency with the others. That will involve changing the same lines in en.json and the app code

The keys specifically, not the values

I think these should be lowercase, for the sake of consistency with the others. That will involve changing the same lines in `en.json` and the app code The keys specifically, not the values

Vutukuri15 (Migrated from github.com) reviewed 2025-03-04 00:29:49 -08:00

i18n/es.json

Vutukuri15 (Migrated from github.com) commented

2025-03-04 00:29:49 -08:00

Thank you for the feedback and suggestions! You're right about the missing update in package.json—I’ll make sure to add the new i18n dependencies.

I appreciate your insights, and I’ll definitely keep them in mind for future work.

Thanks for pointing that out! I’ve updated the keys to lowercase in es.json, en.json and the app code for consistency.

Thank you for the feedback and suggestions! You're right about the missing update in package.json—I’ll make sure to add the new i18n dependencies. I appreciate your insights, and I’ll definitely keep them in mind for future work. Thanks for pointing that out! I’ve updated the keys to lowercase in es.json, en.json and the app code for consistency.