Implement BuyInAmount Selector Component (Issue #3) #15

2025-02-09T15:56:16-08:00

Vutukuri15 commented

2025-02-09 15:56:16 -08:00

(Migrated from github.com)

This addresses the requirement specified in Issue #3.

This PR introduces a new BuyInSelector component for selecting and entering the buy-in amount for the game. The component allows users to choose a predefined buy-in amount or enter a custom value. It includes validation to ensure valid amounts are selected. It also includes corresponding unit tests to ensure the functionality works as expected.

This addresses the requirement specified in Issue #3. This PR introduces a new BuyInSelector component for selecting and entering the buy-in amount for the game. The component allows users to choose a predefined buy-in amount or enter a custom value. It includes validation to ensure valid amounts are selected. It also includes corresponding unit tests to ensure the functionality works as expected.

MantashaNoyela (Migrated from github.com) reviewed 2025-02-09 15:56:16 -08:00

djwesty (Migrated from github.com) requested changes 2025-02-09 19:39:24 -08:00

djwesty (Migrated from github.com) left a comment

Looks good overall

I don't think we need this level of commenting going forward, based on the clean code discussions from class
In the future, when component side-effect logic gets more complicated, keep in mind when to use useMemo, useCallback, etc. I don't think your logic here needs it, but that is a useful article.

Looks good overall * I don't think we need this level of commenting going forward, based on the [clean code](https://software-architecture-ak0.pages.dev/docs/software-engineering/software-design/clean-code#comments) discussions from class * In the future, when component side-effect logic gets more complicated, keep in mind when to use [useMemo, useCallback](https://kentcdodds.com/blog/usememo-and-usecallback), etc. I don't think your logic here needs it, but that is a useful article.

components/BuyInSelector.tsx

djwesty (Migrated from github.com) commented

2025-02-09 19:35:52 -08:00

I have a couple concerns with hardcoding this reference here.

The link is a dependency of our app. If it breaks one day or the user has no network, could be a bad experience
Though you may know its ok, I'm not sure if we have the rights to use this for our app.

One approach would be to save and render the image locally in our app. This solves point 1
Another approach would be to use some public icons, like @expo/vector-icons. If there is a good match, that addresses point 1 and 2.

This should be fixed, but up to you on your approach. You can resolve this thread when done

I have a couple concerns with hardcoding this reference here. * The link is a dependency of our app. If it breaks one day or the user has no network, could be a bad experience * Though you may know its ok, I'm not sure if we have the rights to use this for our app. One approach would be to save and render the image locally in our app. This solves point 1 Another approach would be to use some public icons, like [@expo/vector-icons](https://icons.expo.fyi/Index). If there is a good match, that addresses point 1 and 2. This should be fixed, but up to you on your approach. You can resolve this thread when done

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

components/BuyInSelector.tsx

Vutukuri15 (Migrated from github.com) commented

2025-02-09 20:11:27 -08:00

Thanks for the feedback! I'll check local storage or @expo/vector-icons and update once fixed.

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

components/BuyInSelector.tsx

Vutukuri15 (Migrated from github.com) commented

2025-02-09 21:28:07 -08:00

I've replaced the external image link with the MaterialIcons from @expo/vector-icons, which addresses both the dependency issue and the rights concern.
Let me know if there's anything else to modify.

I've replaced the external image link with the MaterialIcons from @expo/vector-icons, which addresses both the dependency issue and the rights concern. Let me know if there's anything else to modify.