Changed Styles and Screen Size for Mobile Device #59

2025-03-11T06:48:00-07:00

MantashaNoyela commented

2025-03-11 06:48:00 -07:00

(Migrated from github.com)

Implemented a light gray mode instead of dark mode, along with a toggle button to switch between light and gray themes. The PokerAppUi.tsx component was created to manage the app’s layout and background themes properly. Updated the index.tsx file to make the app fully functional, responsive, and aligned with a clean, professional UI suitable for mobile devices.

Vutukuri15 (Migrated from github.com) reviewed 2025-03-11 06:48:00 -07:00

djwesty commented

2025-03-11 11:41:52 -07:00

(Migrated from github.com)

will review soon

djwesty (Migrated from github.com) requested changes 2025-03-11 15:30:39 -07:00

app/index.tsx

djwesty (Migrated from github.com) commented

2025-03-11 15:11:36 -07:00

Keeping track of the color mode ("color scheme") as a state variable here in index.tsx is unfortunately an approach which adds complexity and reduces maintainability. Here are a couple reasons

Any component or container which is a child of index, must be passed the dark mode prop. This includes all uses of the Button which now have the darkMode prop. However, it seems the app buttons are not actually taking this prop in at the moment, which directly highlights the issue - it is easy to forget to utilize all the props 100% of the time and keep everything consistent. The experience now seems to be gray buttons on a gray background when using the mode
Parent components do not have easy access to this mode. That primarily includes the Stack/Layout component in _layout.tsx and would further apply to things like the header bar

Expo has a native mechanism to help with this issue. I mentioned these docs on PR #18, but I can elaborate a bit.

The useColorScheme hook, can be utilize in any components which cares to know the current color scheme and adjust accordingly
The setColorScheme function can be used to set the colorScheme

I feel this should be fixed to use the best approach. I am happy to help if this is a technical challenge, so please let me know if so

Keeping track of the color mode ("color scheme") as a state variable here in `index.tsx` is unfortunately an approach which adds complexity and reduces maintainability. Here are a couple reasons * Any component or container which is a child of index, must be passed the dark mode prop. This includes all uses of the `Button` which now have the `darkMode` prop. However, it seems the app buttons are not actually taking this prop in at the moment, which directly highlights the issue - it is easy to forget to utilize all the props 100% of the time and keep everything consistent. The experience now seems to be gray buttons on a gray background when using the mode * Parent components do not have easy access to this mode. That primarily includes the Stack/Layout component in `_layout.tsx` and would further apply to things like the header bar Expo has a native mechanism to help with this issue. I [mentioned](https://github.com/djwesty/poker-chips-helper/issues/18#issuecomment-2706855094) these docs on PR #18, but I can elaborate a bit. * The [useColorScheme](https://docs.expo.dev/develop/user-interface/color-themes/#detect-the-color-scheme) hook, can be utilize in any components which cares to know the current color scheme and adjust accordingly * The [setColorScheme](https://reactnative.dev/docs/appearance?guide=web#setcolorscheme) function can be used to set the `colorScheme` I feel this should be fixed to use the best approach. I am happy to help if this is a technical challenge, so please let me know if so

app/index.tsx

						
				@ -94,2 +83,4 @@

				    } else {

				      Alert.alert(i18n.t("info"), i18n.t("no_saved_state_found"));

				    }

				  };

djwesty (Migrated from github.com) commented

2025-03-11 14:20:54 -07:00

I see you added atleast 3 new tranlated texts here, but it does not look like there are new additions to en.json and es.json.
I think this will result in an issue where the labels appear incorrect. Please confirm this and fix if necessary by adding the translations. This also applies to the title label appearance above
You may resolve this

I see you added atleast 3 new tranlated texts here, but it does not look like there are new additions to en.json and es.json. I think this will result in an issue where the labels appear incorrect. Please confirm this and fix if necessary by adding the translations. This also applies to the title label `appearance` above You may resolve this

djwesty (Migrated from github.com) commented

2025-03-11 14:46:09 -07:00

Changing the color theme to be feels a bit like Setting, so the user may choose to do once and is unlikely to often interact with it.
Placing the Section for this feature prominently at the top and having it always be visible and and forcing the user to scroll beyond it for all interactions of the app may not be an ideal user experience.

I am not a UI/UX expert though. However, if you agree with what I am saying, you may choose to move this section into the toggable Settings menu just below. In that case you can make the change and resolve this.

If the way you have it is intentional please let me know

Changing the color theme to be feels a bit like Setting, so the user may choose to do once and is unlikely to often interact with it. Placing the Section for this feature prominently at the top and having it always be visible and and forcing the user to scroll beyond it for all interactions of the app may not be an ideal user experience. I am not a UI/UX expert though. However, if you agree with what I am saying, you may choose to move this section into the toggable `Settings` menu just below. In that case you can make the change and resolve this. If the way you have it is intentional please let me know

containers/Button.tsx

djwesty (Migrated from github.com) commented

2025-03-11 14:33:07 -07:00

Can you run npx tsc and confirm that this change, and any other new changes are not introducing typescript errors? It appears from my end that they might be. If so, they should be fixed please. However, please see my other comment about prop passing for a color scheme which may make some of these typescript errors redundant

Free to resolve.

Can you run `npx tsc` and confirm that this change, and any other new changes are not introducing typescript errors? It appears from my end that they might be. If so, they should be fixed please. However, please see my other comment about prop passing for a color scheme which may make some of these typescript errors redundant Free to resolve.

containers/PokerAppUi.tsx

djwesty (Migrated from github.com) commented

2025-03-11 15:24:22 -07:00

I may be wrong, but it seems like the purpose of this component is to be a wrapper of sorts for stylistic purposes, to serve the global app layout.

If that is correct, consider that expos stack navigation system provides a purpose built and best practice system to do this universally, and reduce complexity by not needing wrapper components.

For example, our Apps <Stack> (as implemented in _layout.tsx) can take a screen options parameter called contentStyle. This is an object which can contain a backgroundColor. That would be the expo way to set the background color for our app, even dynamically. The <Stack> props are powerfull and can take all sorts of nested style options for the main app content, header, title, etc.

We can discuss this in sync to make sure we are all on the right path

I may be wrong, but it seems like the purpose of this component is to be a wrapper of sorts for stylistic purposes, to serve the global app layout. If that is correct, consider that [expos stack navigation](https://docs.expo.dev/router/advanced/stack/#configure-header-bar) system provides a purpose built and best practice system to do this universally, and reduce complexity by not needing wrapper components. For example, our Apps `<Stack>` (as implemented in `_layout.tsx`) can take a screen options parameter called `contentStyle`. This is an object which can contain a `backgroundColor`. That would be the expo way to set the background color for our app, even dynamically. The `<Stack>` props are powerfull and can take all sorts of nested style options for the main app content, header, title, etc. We can discuss this in sync to make sure we are all on the right path

djwesty (Migrated from github.com) commented

2025-03-11 15:25:24 -07:00

Do you know what the status bar is doing for the app?

util/tests/PersistentState.test.ts

						
				@ -0,0 +10,4 @@

				  getItem: jest.fn(),

				}));

				describe("PersistentState.ts", () => {

djwesty (Migrated from github.com) commented

2025-03-11 14:26:25 -07:00

Not related to this file/line, but I checked out the branch and ran npm t It seems that possibly some existing tests have broken with these changes.
Can you confirm this, and fix up the pre-existing tests as necessary so they all pass?

Free to resolve when done

Not related to this file/line, but I checked out the branch and ran `npm t` It seems that possibly some existing tests have broken with these changes. Can you confirm this, and fix up the pre-existing tests as necessary so they all pass? Free to resolve when done

djwesty commented

2025-03-11 15:40:33 -07:00

(Migrated from github.com)

It seems like this PR is addressing two different documented issues (#18 and #22), and the one undocumented issue of the non-component code being in component files.

for future reference, I suggest opening different PRs for different issues, and ensureing the issues are created prior to the PR

It seems like this PR is addressing two different documented issues (#18 and #22), and the one undocumented issue of the non-component code being in component files. for future reference, I suggest opening different PRs for different issues, and ensureing the issues are created prior to the PR

MantashaNoyela (Migrated from github.com) reviewed 2025-03-13 01:07:46 -07:00

containers/PokerAppUi.tsx

MantashaNoyela (Migrated from github.com) commented

2025-03-13 01:07:46 -07:00

The StatusBar is used to control the appearance of the mobile device's status bar when the app is running.

MantashaNoyela (Migrated from github.com) reviewed 2025-03-13 01:22:21 -07:00

util/tests/PersistentState.test.ts

						
				@ -0,0 +10,4 @@

				  getItem: jest.fn(),

				}));

				describe("PersistentState.ts", () => {

MantashaNoyela (Migrated from github.com) commented

2025-03-13 01:22:21 -07:00

It is working without any error on my machine.

MantashaNoyela (Migrated from github.com) reviewed 2025-03-13 01:23:31 -07:00

containers/Button.tsx

MantashaNoyela (Migrated from github.com) commented

2025-03-13 01:23:31 -07:00

fixed these.

MantashaNoyela commented

2025-03-17 18:23:21 -07:00

(Migrated from github.com)

I am having issues while seeing the app on mobile device. So, I could not resolve the issue that the app is not fitting perfectly on smaller devices. @djwesty, would you please resolve the issue? Thank you.

MantashaNoyela (Migrated from github.com) reviewed 2025-03-17 18:24:20 -07:00

containers/PokerAppUi.tsx

MantashaNoyela (Migrated from github.com) commented

2025-03-17 18:24:20 -07:00

I believe this issue is solved

djwesty commented

2025-03-17 18:25:12 -07:00

(Migrated from github.com)

Sure thing, I will test and commit changes as needed 👍

👍 1

djwesty commented

2025-03-18 00:00:13 -07:00

(Migrated from github.com)

I've pushed commits with the following changes to ensure the app looks good on a mobile device and is demo ready.

Changes from the gray mode to dark mode: The gray on gray color pallet was not ideal on a device
Remove extra UI wrapper layer: On a mobile device, this added an extra scroll bar, and pushed all the content off to the right.
Use hooks to get/set color mode. This allows the users phone OS to try to automatically pick the mode, and eliminates the need to prop drill darkmode and manually manage darkmode state. This also allows darkmode to apply consistently to the higher level layout like the title bar
Make save/load buttons smaller so they do not run off the screen
Containerize React native picker to reduce style redundancy and make the picker round + consistent with the buttons
Centralize the color pallet to ensure consistent colors and module theme.

I've pushed commits with the following changes to ensure the app looks good on a mobile device and is demo ready. - Changes from the gray mode to dark mode: The gray on gray color pallet was not ideal on a device - Remove extra UI wrapper layer: On a mobile device, this added an extra scroll bar, and pushed all the content off to the right. - Use hooks to get/set color mode. This allows the users phone OS to try to automatically pick the mode, and eliminates the need to prop drill darkmode and manually manage darkmode state. This also allows darkmode to apply consistently to the higher level layout like the title bar - Make save/load buttons smaller so they do not run off the screen - Containerize React native picker to reduce style redundancy and make the picker round + consistent with the buttons - Centralize the color pallet to ensure consistent colors and module theme.

djwesty (Migrated from github.com) approved these changes 2025-03-18 00:00:23 -07:00

Vutukuri15 commented

2025-03-18 00:06:59 -07:00

(Migrated from github.com)

Great work on refining the UI! The switch to dark mode and centralizing the color palette definitely help with consistency.
Thanks for making these improvements!

Great work on refining the UI! The switch to dark mode and centralizing the color palette definitely help with consistency. Thanks for making these improvements!

Sign in to join this conversation.

No reviewers

Vutukuri15

djwesty