Skip to content
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

Merged
merged 20 commits into from
Oct 20, 2024
Merged

129 make navbar dynamic #143

merged 20 commits into from
Oct 20, 2024

Conversation

h4yleysh4rpe
Copy link
Contributor

@h4yleysh4rpe h4yleysh4rpe commented Oct 10, 2024

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?

  • Created two new components - server-side SmallNavbarFetcher and client-side SmallNavbar.
  • Created arrow svg's which live in the svg folder
  • layout.tsx now contains both header components and renders conditionally based on screeen width
  • Small navbar now shows up instead of the header for screen widths < xs (size in tailwind.config).

How To Review

Can review commit wise. Please check that all commit messages are satisfied as well as -

  • no weird scaling issues, especially for reasonable mobile widths
  • all pages are able to be navigated to

Notes

  • I accidentally called the branch 129_make-branch-dynamic instead of navbar so just keep that in mind sorry!
  • There is quite a long delay time sometimes when clicking link -> moving to new page, so just keep that in mind

@h4yleysh4rpe
Copy link
Contributor Author

@Oculux314 @belleyong @jessicazeng13 just a cheeky little nudge in case you did not see this! No pressure I know it is busy times :)

Copy link
Contributor

@Oculux314 Oculux314 left a 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?)

@h4yleysh4rpe
Copy link
Contributor Author

h4yleysh4rpe commented Oct 20, 2024

Have updated my code wrt Nate's requests so feel free to review when u get a chance!

@belleyong
Copy link
Contributor

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!

Copy link
Contributor

@belleyong belleyong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Contributor

@Oculux314 Oculux314 left a 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):

  • Projects after members
    image
    image
    Would be nice to have the order of the projects tab be the same across the two navbars
  • Merge the small_navbar and header folders
  • Filename: headerData.tsx -> headerTypes.tsx (headerData is ambiguous)

@Oculux314
Copy link
Contributor

Oh also I think the white bar at the very top is being caused by an outline in layout.tsx xD

Copy link
Contributor

@jessicazeng13 jessicazeng13 left a 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?

@h4yleysh4rpe h4yleysh4rpe merged commit b93725a into main Oct 20, 2024
@h4yleysh4rpe h4yleysh4rpe deleted the 129_make-branch-dynamic branch October 20, 2024 23:24
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.

Make Navbar dynamic
4 participants