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

[feat] implements the navbar #65

Merged
merged 37 commits into from
May 20, 2024
Merged

[feat] implements the navbar #65

merged 37 commits into from
May 20, 2024

Conversation

kevin3656
Copy link
Contributor

@kevin3656 kevin3656 commented Mar 5, 2024

🎋 Description

🌴 What's new in this PR

  • Implemented Nav Bar Component in components. icon redirects to homepage. cases, limited cases and language support redirect to cases page for now.
  • changed authbuttons implementation from cases page to navbar.
  • added ijp logo to icon in assets
  • Added NavBar Component to cases page, settings page and auth layout.tsx

🌲 Screenshots

image

🌳 How to review

From the homepage you should be able to navigate across pages using nav bar, when logged in nav bar should change to show profile instead of log in/sign up buttons
can check implementation on src/components/NavBar

🌱 Next steps

  • update hover effect on links
  • fix backlink on settings page(currently overlaps with navbar)

🔗 Relevant Links

ℹ️ Online sources

https://www.youtube.com/watch?v=SLfhMt5OUPI

🪴 Related PRs

CC: @varortz

Copy link

linear bot commented Mar 5, 2024

IJP-109 Navigation Bar

src/components/NavBar/style.tsx Outdated Show resolved Hide resolved
src/components/NavBar/style.tsx Outdated Show resolved Hide resolved
src/components/NavBar/style.tsx Outdated Show resolved Hide resolved
src/components/NavBar/style.tsx Outdated Show resolved Hide resolved
src/components/NavBar/style.tsx Outdated Show resolved Hide resolved
src/components/ProfileButton.ts Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/ProfileButton.ts Outdated Show resolved Hide resolved
src/components/NavBar/style.tsx Outdated Show resolved Hide resolved
src/app/settings/page.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@ccatherinetan ccatherinetan left a comment

Choose a reason for hiding this comment

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

the nav bar looks rlly good! I couldn't fully review this because the cases page is erroring for me, so i couldn't check the functionality of clicking on cases/language support etc on the navbar... is anyone else having the same problem? (the font also doesn't show up for me? maybe that's a me thing)

i caught 1 place where the navbar component was erroneously added (in (auth)/email-verified). overall, good job! i'll check back in once we figure out the error w/ the cases page T^T
Screen Shot 2024-03-06 at 9 59 57 PM

@varortz varortz requested a review from kyrenetam March 8, 2024 03:44
@jinkang-0
Copy link
Contributor

jinkang-0 commented Mar 8, 2024

To reduce redundancy and keep merge errors at a minimum, I would recommend placing the NavBar component in app/layout.tsx.

This may sound like lunacy, but hear me out.

Since the NavBar will have to be a client component anyways (to access the profile context), it will also be able to access the router, and by extension, the current URL / path of the browser. With this information, we can:

  1. Keep the logic of determining which tab is "active" in the component itself (no need to pass props)
  2. Determine whether to show or hide the NavBar depending on the URL
  3. Easily update the logic of the two points above inside the NavBar component if we ever decide to change it (without affecting other files)

This may bring a significant change to what you have right now, but I would highly encourage adopting this paradigm as it is used commonly in practice, due to it's ability to take advantage of the layout provided by Next.js to avoid re-rendering the NavBar each time we visit a new page.

And also make our lives easier. Merge conflicts bad.

@jinkang-0 jinkang-0 changed the title feat: first implementation of nav bar [feat] first implementation of nav bar Mar 20, 2024
@kevin3656 kevin3656 force-pushed the kevin/ijp-109-navigation-bar branch from bd2588e to cb0c41a Compare April 19, 2024 03:18
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/NavBar/style.ts Outdated Show resolved Hide resolved
src/components/NavBar/style.ts Outdated Show resolved Hide resolved
src/components/NavBar/style.ts Outdated Show resolved Hide resolved
@varortz varortz changed the title [feat] first implementation of nav bar [feat] implements the navbar May 6, 2024
@kevin3656 kevin3656 force-pushed the kevin/ijp-109-navigation-bar branch from 15b5a18 to 7032e96 Compare May 15, 2024 22:03
src/components/Button.tsx Outdated Show resolved Hide resolved
assets/icons/Icon.tsx Outdated Show resolved Hide resolved
assets/icons/logo.svg Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
src/components/SmallerButton.tsx Outdated Show resolved Hide resolved
src/components/NavBar/style.ts Outdated Show resolved Hide resolved
src/components/NavBar/style.ts Outdated Show resolved Hide resolved
src/components/Button.tsx Outdated Show resolved Hide resolved
@jinkang-0
Copy link
Contributor

looks good kevin! should be good after addressing some minor nits and prettier complaints!
one note: don't forget to remove the back link on the settings page as well!

src/components/Buttons.tsx Outdated Show resolved Hide resolved
src/lib/icons.tsx Outdated Show resolved Hide resolved
src/components/NavBar/index.tsx Outdated Show resolved Hide resolved
@varortz varortz force-pushed the kevin/ijp-109-navigation-bar branch from cf2d903 to 0a10848 Compare May 20, 2024 04:33
Copy link
Contributor

@jinkang-0 jinkang-0 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
Collaborator

@varortz varortz left a comment

Choose a reason for hiding this comment

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

NAVBAR LOOKS AMAZING!!! Thank you Kevin for working on this throughout semester, you killed it :)

@varortz varortz merged commit 3f76030 into main May 20, 2024
2 checks passed
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.

4 participants