Add responsive Navbar based on Figma design #15
Conversation
|
@heshanthenura Please replace the logo with the SVG one since it fits better in all aspect ratios. Also, add some top margin to the navbar for better spacing. Other than that, everything looks great! |
Sure ayya I’ll do |
|
Seniru
left a comment
Seniru
left a comment
There was a problem hiding this comment.
Because you requested a review- looks ok, maybe a little padding from either sides (left and right) if we are going exactly with the figma design
|
Hi all, as a practice please attach screenshots of what you have developed in desktop & mobile views for faster reviews 🙌 |
There was a problem hiding this comment.
is it possible to move this to a folder inside assets?
ex:
- assets
- images
- logos
- logo-dark.svg
- icons
There was a problem hiding this comment.
yes ayya Ill do
src/app/board/page.tsx
Outdated
There was a problem hiding this comment.
Is this component necessary in this PR?
There was a problem hiding this comment.
my bad. forgot to switch branch ill remove
src/components/NavBar.tsx
Outdated
|
|
||
| return ( | ||
| <div className="w-screen flex items-center justify-center md:mt-[25px]"> | ||
| <nav className="md:rounded-[24px] bg-[linear-gradient(90deg,rgba(141,198,255,0.61)_0%,rgba(173,101,255,0.61)_100%)] p-[2px] w-screen md:w-[70%] md:flex md:items-center md:justify-center"> |
There was a problem hiding this comment.
is this the purple-blue gradient that we'll be using throughout the site? if so pls move the gradient style to the tailwind config and we can reuse it 😉
src/components/NavBar.tsx
Outdated
| open ? 'flex' : 'hidden md:flex' | ||
| }`} | ||
| > | ||
| <Link href={'#'}>Home</Link> |
There was a problem hiding this comment.
These links and any texts in the navbar should be loaded from a separate file from within the codebase, for ease.
ex:
lib/data/navbar.ts and include the data in it as an object.
ex:
navbarData
- logo
- imageUrl
- link
- links (array)
- text
- linkUrl
something like this
| @@ -0,0 +1,58 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
Also using use client on the top of the component makes the whole component client-side, which misses the whole point of using SSR, so let's try to use use client in the smallest components possible .
Ex: if only a button needs to have local states, then use use client in the button component separately and use it here
src/components/NavBar.tsx
Outdated
| <Image | ||
| src="/assets/logo-dark.svg" | ||
| alt="Logo" | ||
| width={0} |
There was a problem hiding this comment.
we'll have to use width & height rather than setting it to zero 😉
Please refer: https://nextjs.org/docs/app/api-reference/components/image#width-and-height
| /> | ||
|
|
||
| <div className="md:hidden"> | ||
| <button onClick={() => setOpen(!open)}> |
There was a problem hiding this comment.
can we have a reusable button component with some variants?
There was a problem hiding this comment.
I added a Button component in my own PR. Maybe we can all use that.
|
4 participants
This PR addresses issue #3 and implements the following changes: