Implemented Currency Selector # 34 #46

2025-02-28T11:22:26-08:00

Vutukuri15 commented

2025-02-28 11:22:26 -08:00

(Migrated from github.com)

This PR introduces a currency selection feature for the poker game app, allowing players to change the currency symbol displayed for chip values and buy-in amounts. The main updates include the addition of a dropdown or setting where users can select a currency symbol (e.g., $, €, ₹, £). Upon selection, all displayed monetary values will automatically update to reflect the chosen currency.

MantashaNoyela (Migrated from github.com) reviewed 2025-02-28 11:22:26 -08:00

djwesty (Migrated from github.com) approved these changes 2025-02-28 17:12:17 -08:00

djwesty (Migrated from github.com) left a comment

Good work. There's just a few minor things.

Thank you for taking the time to fix all the existing tests

Good work. There's just a few minor things. Thank you for taking the time to fix all the existing tests

app/index.tsx

djwesty (Migrated from github.com) commented

2025-02-28 16:50:11 -08:00

The settings button seems like a good idea. I have a few thoughts

Can this button go in the header bar? If so It would save vertical space on the app. Experiment with the prop headerRight in the RootLayout Component screenoptions in _layout.tsx
You could use the Cog icon instead of the label "Settings" if you need to to save horizontal space. Using icons instead of text when possible is also a generally good idea from a language support perspective.

The settings button seems like a good idea. I have a few thoughts - Can this button go in the header bar? If so It would save vertical space on the app. Experiment with the prop `headerRight` in the RootLayout Component `screenoptions` in `_layout.tsx` - You could use the Cog icon instead of the label "Settings" if you need to to save horizontal space. Using icons instead of text when possible is also a generally good idea from a language support perspective.

djwesty (Migrated from github.com) commented

2025-02-28 16:50:33 -08:00

This seems like a possible regression. I think this is not present currently on the main branch.

Please confirm and remove this if so

This seems like a possible regression. I think this is not present currently on the main branch. Please confirm and remove this if so

djwesty (Migrated from github.com) commented

2025-02-28 17:03:38 -08:00

I notice you have a title of the setting here, as well as a label for "Select Currency" within the component itself. I think this combination is a bit wordy, and arguably redundant.

If you want to have a title, I say would say get rid of this one and keep the other.
Another idea is to have no labels at all. I would support this by saying seeing a currency option in a dropdown menu is a pretty intuitive user experience, especially after they must click settings.

I think you should make one of the two above changes, but the decision is yours.

I notice you have a title of the setting here, as well as a label for "Select Currency" within the component itself. I think this combination is a bit wordy, and arguably redundant. * If you want to have a title, I say would say get rid of this one and keep the other. * Another idea is to have no labels at all. I would support this by saying seeing a currency option in a dropdown menu is a pretty intuitive user experience, especially after they must click settings. I think you should make one of the two above changes, but the decision is yours.

components/BuyInSelector.tsx

						
				@ -10,11 +10,15 @@ import { MaterialIcons } from "@expo/vector-icons";

				interface BuyInSelectorProps {

				  setBuyInAmount: React.Dispatch<React.SetStateAction<number>>;

				  selectedCurrency: string; // Accept selectedCurrency as a prop

djwesty (Migrated from github.com) commented

2025-02-28 17:10:16 -08:00

For a global setting like this, contexts might be a better choice.

If our app was to grow and gain complexity, we would want to avoid the inevitable 'prop drilling' as described in that article.

However, you do not need to change this. Just be aware of the context feature for future reference and personal learning. Passing as props is ok for our circumstances given the scope of our app.

For a global setting like this, [contexts](https://react.dev/learn/passing-data-deeply-with-context) might be a better choice. If our app was to grow and gain complexity, we would want to avoid the inevitable 'prop drilling' as described in that article. However, you do not need to change this. Just be aware of the context feature for future reference and personal learning. Passing as props is ok for our circumstances given the scope of our app.

components/CurrencySelector.tsx

						
				@ -0,0 +20,4 @@

				        style={styles.picker}

				        testID="currency-picker" // ✅ Add testID here

				      >

				        <Picker.Item label="USD ($)" value="$" />

djwesty (Migrated from github.com) commented

2025-02-28 16:57:24 -08:00

One thing about currencies is that multiple currencies sometimes use the same symbol (example, Mexican Peso also uses the $ sign).
I think what you have is good for our app though and you do not need to change anything. This is just an interesting fact.

One thing about currencies is that multiple currencies sometimes use the same symbol (example, Mexican Peso also uses the `$` sign). I think what you have is good for our app though and you do not need to change anything. This is just an interesting fact.

Vutukuri15 (Migrated from github.com) reviewed 2025-02-28 18:31:11 -08:00

components/BuyInSelector.tsx

						
				@ -10,11 +10,15 @@ import { MaterialIcons } from "@expo/vector-icons";

				interface BuyInSelectorProps {

				  setBuyInAmount: React.Dispatch<React.SetStateAction<number>>;

				  selectedCurrency: string; // Accept selectedCurrency as a prop

Vutukuri15 (Migrated from github.com) commented

2025-02-28 18:31:11 -08:00

Thanks for the feedback! I’ll keep that in mind for future improvements.

Vutukuri15 (Migrated from github.com) reviewed 2025-02-28 18:52:34 -08:00

components/CurrencySelector.tsx

						
				@ -0,0 +20,4 @@

				        style={styles.picker}

				        testID="currency-picker" // ✅ Add testID here

				      >

				        <Picker.Item label="USD ($)" value="$" />

Vutukuri15 (Migrated from github.com) commented

2025-02-28 18:52:34 -08:00

Yes, good to know! I think we're good with the current approach, as you said

Vutukuri15 (Migrated from github.com) reviewed 2025-02-28 19:05:09 -08:00

app/index.tsx

Vutukuri15 (Migrated from github.com) commented

2025-02-28 19:05:08 -08:00

I removed title Currency Selector

djwesty (Migrated from github.com) approved these changes 2025-02-28 19:49:19 -08:00

package.json

djwesty (Migrated from github.com) commented

2025-02-28 19:49:09 -08:00

I think we are already using@expo/vector-icons for some icons. We should avoid pulling in a separate new icon package unless the is a good justified reason. I suggest finding a suitable icon from there instead and removing this new dependency, or leave a quick note here about the reason for needing this new package please.

Free to resolve.

I think we are already using`@expo/vector-icons` for some icons. We should avoid pulling in a separate new icon package unless the is a good justified reason. I suggest finding a suitable icon from there instead and removing this new dependency, or leave a quick note here about the reason for needing this new package please. Free to resolve.