-
Notifications
You must be signed in to change notification settings - Fork 1
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
129 make navbar dynamic #143
Conversation
@Oculux314 @belleyong @jessicazeng13 just a cheeky little nudge in case you did not see this! No pressure I know it is busy times :) |
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.
Sorry for the late review (and thanks for the reminder)! This looks great, and it'll be really helpful for everyone viewing the website on a phone. (The animations are a nice touch btw)
Here's the changes I suggest before merging:
- There's a lot of functionality shared between this and the regular navbar! Would you be able to extract the getHeaderData() function and HeaderData type into their own files? (use your version of the function :D)
- Could you make the arrows clickable? At the moment clicking the arrows themselves does nothing 🙃
- Consider keeping just one version of the arrow svg, and extract out the other icons into separate components) (Those icons are awesome btw! How'd you generate them?)
Have updated my code wrt Nate's requests so feel free to review when u get a chance! |
@h4yleysh4rpe I'm unable to run localhost without changing the code as I'm currently working with strapi and there is things that doesn't match up with strapi so im unable to physically test your amazing changes but I had a look at the code and it looks amazing! |
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.
lgtm!
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.
This looks super awesome, happy to merge! Wow feels so responsive
A few minor things you could consider for brownie marks (optional & feel free to merge straight after I trust):
Oh also I think the white bar at the very top is being caused by an outline in layout.tsx xD |
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 navbar looks all good to me! There still is that little white line up top for me which nate said was a thing in the layouts.tsx file?
Context
Creating a navbar for smaller screen widths, including mobile view. This should not only make it much easier for mobile users to navigate the site, but also help the mobile view compatibility, as the header no longer takes up the same fixed width across the whole page anymore.
Closes #129
What Changed?
How To Review
Can review commit wise. Please check that all commit messages are satisfied as well as -
Notes