Skip to content

Conversation

@dorawengg
Copy link
Collaborator

Added navbar but theres an error in node_modules

@dorawengg dorawengg requested a review from JasonMun7 October 17, 2024 07:37
Copy link
Collaborator

@JasonMun7 JasonMun7 left a 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!


/**
* You can explore the built-in icon families and icons on the web at https://icons.expo.fyi/
*/
Copy link
Collaborator

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 = [
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

}

return (
<>
Copy link
Collaborator

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;


if (route.name === 'index') {
return (
<TouchableOpacity
Copy link
Collaborator

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.

left: screenWidth / 2 - 40, // Center the button horizontally (the add button width is 80px)
}}
>
<Text style={{ color: 'white', fontSize: 40 }}>+</Text>
Copy link
Collaborator

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 (
Copy link
Collaborator

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

@dorawengg dorawengg requested a review from JasonMun7 October 18, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants