-
Notifications
You must be signed in to change notification settings - Fork 0
Nav bar #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
JasonMun7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Dora! This is starting to look awesome! I've left some comments for you read over! If you have any questions don't hesitate to reach out! Other than that great job!
app/(tabs)/_layout.tsx
Outdated
|
|
||
| /** | ||
| * You can explore the built-in icon families and icons on the web at https://icons.expo.fyi/ | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be best to get rid of some of these unnecessary comments that don't help with readability.
|
|
||
| // Drawing the tab shape | ||
| const TabsShape: React.FC<TabsShapeProps> = ({ tabWidth }) => { | ||
| // const points = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saving this code for later? If not I think it'd be best to omit this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its good for visualizing the critical points if we ever want to change it or use the same library for something else later.
components/TabBar.tsx
Outdated
| } | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid using empty fragments (<> </>). If no default route needs to be rendered, use early returns to improve clarity.
Something like if (!['add', 'index', 'profile'].includes(route.name)) return null;
components/TabBar.tsx
Outdated
|
|
||
| if (route.name === 'index') { | ||
| return ( | ||
| <TouchableOpacity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be a good idea to create a reusable component for this Touchable Opacity as it seems that the logic is relatively the same for both the index and profile.
components/TabBar.tsx
Outdated
| left: screenWidth / 2 - 40, // Center the button horizontally (the add button width is 80px) | ||
| }} | ||
| > | ||
| <Text style={{ color: 'white', fontSize: 40 }}>+</Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very Nitpick, the the Add should be a little closer to the bar itself. The + seems to be a little bigger within the figma as well.
|
|
||
| // Add button | ||
| if (route.name === 'add') { | ||
| return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button should be a gradient as seen within the figma. I'd look into expo-gradient to get you started on that.
It's from #D372E5 to #5731D6
Added navbar but theres an error in node_modules