Setting page added #23

madhushree007 · 2024-10-18T15:38:43Z

@giolaq I have added a setting page. Please have a look. If the UI looks ok, I will add language selector on this page.

giolaq · 2024-10-18T17:38:48Z

Thank you, can you link this PR to this issue ?
Attach a couple of the screenshots on the issue too.

It's preferred to include Settings not at the same level of the main navigation paths but inside a page that the user can access from the profile.

giolaq · 2024-10-18T17:36:01Z

app/(drawer)/settings.tsx

+import { useCallback, useState } from 'react';
+import { Direction } from '@bam.tech/lrud';
+
+export default function TVScreen() {


Why is it called TVScreen? I think calling it SettingScreen is more suitable

Fixed! thanks for pointing.

giolaq · 2024-10-24T15:22:23Z

package.json

@@ -37,17 +38,17 @@
    "react-native-reanimated": "~3.10.1",
    "react-native-safe-area-context": "4.10.1",
    "react-native-screens": "3.31.1",
+    "react-native-vector-icons": "^10.2.0",


I think this is the only things added to the package.json, the others modified lines seems just moving lines. Can you commit only a file with this dependency entry?

Yes, This is the only thing added in the package.json. The vector icons has been used in setting.tsx and CustomDrawerContent.tsx

@giolaq I have separated the setting menu in a separate file and imported it in custom drawer. Now setting.tsx and SettingMenu.tsx has icon dependency.

Neha

thank you for your PR. Could you please fill the PR template.

I have added a few comments.

Neha · 2024-10-27T14:38:07Z

app/(drawer)/_layout.tsx

@@ -77,6 +84,7 @@ const useDrawerStyles = function () {
      width: scaledPixels(300),
      backgroundColor: '#2c3e50', // Dark blue background


nitpick: can you please remove the comments for backgroundColor & paddingTop

Neha · 2024-10-27T14:41:13Z

app/(drawer)/settings.tsx

+  );
+}
+
+const useTVStyles = function() {


nitpick: naming convention should be more align towards the setttings rather that "tvstyles"

Neha · 2024-10-27T14:41:46Z

app/(drawer)/settings.tsx

+        textAlign: 'center',
+      },
+      focusedMenuItem: {
+        backgroundColor: '#FFFFFF', 


nitpick: should be lowercase and only 3 chars - #fff

Neha · 2024-10-27T14:42:21Z

app/(drawer)/settings.tsx

+      submenu: {
+        flex: 1, 
+        justifyContent: 'flex-start',
+        paddingVertical: 20,


paddingVertical and paddingHorizontal can be replace by just padding.

Neha · 2024-10-27T14:42:49Z

app/(drawer)/settings.tsx

+        justifyContent: 'space-between',
+      },
+      focusedSubMenuItem: {
+        backgroundColor: '#EEE',


for consistency, can we please make color code to lowercase please

Neha · 2024-10-27T14:50:32Z

components/SettingMenu.tsx

+      fontSize: scaledPixels(32),
+    },
+    menuTextFocused: {
+      color: 'black',


nitpick: same as above.

Neha · 2024-10-27T14:50:57Z

components/SettingMenu.tsx

+      fontSize: scaledPixels(16),
+    },
+    footerText: {
+      color: '#FFF',


please make it lowercase (and where ever the color code is uppercase).

Neha · 2024-10-27T14:52:14Z

app/(drawer)/settings.tsx

+        backgroundColor: '#444',
+        borderRadius: 8,
+      },
+      menuText: {


question: why we have dropped scaledPixels() for this element.

Neha · 2024-10-27T14:53:27Z

components/SettingMenu.tsx

+export default function SettingMenu(props: any) {
+  const router = useRouter();
+  const { isOpen: isMenuOpen, toggleMenu } = useMenuContext();
+  const styles = useDrawerStyles();


this should be right after the function definition (for consistency)

Neha · 2024-10-27T14:54:33Z

components/CustomDrawerContent.tsx

+      minHeight: '90%',
+      flex: 1,
+      marginBottom: scaledPixels(20),
+  },


formatting looks off here. Please format the file.

Neha · 2024-10-29T16:56:25Z

Thank you @madhushree007 for implementing the comments. PR looks great.

I ran the PR at local , here are the few things to take care of:

Web and tvOS: settings font-size is not consistent with other in the left navigation
Web: not able to navigate the options on setting page by keyboard
Web: not able to select any other language beside English
Web: UI is breaking on clicking on "Sound" and "display"

tvOS:

there is extra space on top of settings in left navigation
Navigation between language, sound, and display is little confusing . It sometimes skip the language (white background didn't get visible)

madhushree007 · 2024-10-30T23:21:19Z

@Neha I have made changes according to your comments. I am not able to the PR template. Could you please guide me how and where I can fill PR template?

madhushree007 · 2024-10-30T23:26:26Z

@Neha Here is the screen recording for from tvOS simulator and web browser. I am aware of the issues @giolaq has mentioned. Those issues still needs to be fixed. II’m finding it a bit challenging to manage CSS for both tvOS and web platforms simultaneously. Any suggestions on balancing or streamlining the process would be really helpful.

tvOS.mov

web_browser.mov

Neha · 2024-11-01T19:28:06Z

@Neha I have made changes according to your comments. I am not able to the PR template. Could you please guide me how and where I can fill PR template?

When you raise the PR, it should show you default template for raising PR.

Neha · 2024-11-01T19:29:19Z

@Neha Here is the screen recording for from tvOS simulator and web browser. I am aware of the issues @giolaq has mentioned. Those issues still needs to be fixed. II’m finding it a bit challenging to manage CSS for both tvOS and web platforms simultaneously. Any suggestions on balancing or streamlining the process would be really helpful.

tvOS.mov
web_browser.mov

have you checked Platform.OS?

here is the documention: https://reactnative.dev/docs/platform-specific-code

madhushree007 · 2024-11-18T13:58:54Z

@Neha @giolaq I have added some css fix as per your suggestions :)

Neha

Thank you so much for your amazing work.

The formatting of all files is very off. Can you please fix that. It is hard to understand all changes you made since last review.
I am also not able to run this code on TvOS but it was working till last changes.
On web, the settings font-size and position is not align to the rest of the link. How would you like to keep it? same as rest of the links or different?
Setting pages on the web have alignment issues (please check screenshots).

I have left a few comments please check:

Neha · 2024-10-28T08:20:51Z

app/(drawer)/settings.tsx

+    { id: 'display', label: 'Display' },
+  ];
+
+  const submenus = {


We should move the languages to a separate config file

Neha · 2024-11-19T09:31:34Z

components/CustomDrawerContent.tsx

+								<DefaultFocus key={index}>
+									<SpatialNavigationFocusableView
+										onSelect={() => {
+											console.log(item.name);


please remove the console

Neha · 2024-11-19T09:31:48Z

components/CustomDrawerContent.tsx

+								<SpatialNavigationFocusableView
+									key={index}
+									onSelect={() => {
+										console.log(item.name);


please remove the console

Neha · 2024-11-19T09:33:05Z

components/SettingMenu.tsx

+			fontSize: scaledPixels(16),
+		},
+		footerText: {
+			color: '#FFF',


should be lowercase. Have the consistency in styling. Either have hexacode or string.

Neha · 2024-11-19T09:33:13Z

components/SettingMenu.tsx

+		},
+		cogIcon: {
+			paddingRight: scaledPixels(6),
+			color: '#FFF',


same as above

Neha · 2024-11-19T09:33:44Z

package.json

@@ -23,6 +23,7 @@
    "expo-build-properties": "~0.12.3",
    "expo-constants": "~16.0.2",
    "expo-font": "~12.0.7",
+    "expo-linear-gradient": "~13.0.2",


are we using linear gradient in settings page?

madhushree007 · 2024-11-19T11:53:37Z

Thanks for your comments @Neha

For the formatting issue - I am using Prettier in VS code that might have different setting from yours because code looks well formatted in my code editor.

I have just checked and I can confirm that It is working as expected in TvOS. I am not sure why it is not working at your side. I can see there is a conflict in the PR but I am not able to resolve the conflict. I did try to pull the branch but I am not authorised.

Setting menu is separated from the rest of the menu as suggested by @giolaq. It is supposed to be smaller in font size and separated from Drawer menu.

I had a look at the Screenshot for web view and I think this is the expected behaviour because we do not have any data for other menus so we are not displaying the submenu for "Sound" and "Display". These are only placeholder. Here is the SS from my web view.

Thanks.

Setting page added

ddcb738

giolaq marked this pull request as draft October 18, 2024 17:41

setting menu separated from main menu and setting page formatting done

d65d590

madhushree007 marked this pull request as ready for review October 24, 2024 15:29

giolaq requested changes Oct 24, 2024

View reviewed changes

Madhushree Gupta added 2 commits October 24, 2024 18:01

component name changed

bcf5116

setting menu separated from xustom drawer

c024783

Neha reviewed Oct 27, 2024

View reviewed changes

Madhushree Gupta added 2 commits October 27, 2024 17:59

comments removed

3948159

comments removed

6cfe260

css fix added

d0caf4a

Neha reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting page added #23

Setting page added #23

madhushree007 commented Oct 18, 2024

giolaq commented Oct 18, 2024

giolaq Oct 18, 2024

madhushree007 Oct 24, 2024

giolaq Oct 24, 2024

madhushree007 Oct 24, 2024

madhushree007 Oct 24, 2024

Neha left a comment

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha Oct 27, 2024

Neha commented Oct 29, 2024 •

edited

Loading

madhushree007 commented Oct 30, 2024

madhushree007 commented Oct 30, 2024

Neha commented Nov 1, 2024

Neha commented Nov 1, 2024

madhushree007 commented Nov 18, 2024

Neha left a comment

Neha Oct 28, 2024

Neha Nov 19, 2024

Neha Nov 19, 2024

Neha Nov 19, 2024

Neha Nov 19, 2024

Neha Nov 19, 2024

madhushree007 commented Nov 19, 2024

		@@ -77,6 +84,7 @@ const useDrawerStyles = function () {
		width: scaledPixels(300),
		backgroundColor: '#2c3e50', // Dark blue background

Setting page added #23

Are you sure you want to change the base?

Setting page added #23

Conversation

madhushree007 commented Oct 18, 2024

giolaq commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neha commented Oct 29, 2024 • edited Loading

madhushree007 commented Oct 30, 2024

madhushree007 commented Oct 30, 2024

Neha commented Nov 1, 2024

Neha commented Nov 1, 2024

madhushree007 commented Nov 18, 2024

Neha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhushree007 commented Nov 19, 2024

Neha commented Oct 29, 2024 •

edited

Loading