Feature/navbar#35
Conversation
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Overall the navbar looks really good! The scroll behaviour looks great. Nice work :D
Just a couple points
- maybe it's just on my screen but the navbar overall looks a bit too small in comparison to the hero / rest of the website.
- on hover of the different pages, only the text should change colour. Rn the surrounding box is also changing colour
- we should add some sort of visual indicator in the navbar to show the current page we are on, could be as simple as just changing the colour to be yellow / underlined. Will discuss more in meeting with designer
There was a problem hiding this comment.
Could you explain the changes in this file? If it's unrelated to navbar it may be better suited to remove them from this PR and put the changes in a different PR so it's more clear
There was a problem hiding this comment.
pnpm required those built scripts to be enabled when I tried running linting and formatting. I had to add the pnpm workspace file to prettier ignore as prettier was throwing errors because of the file
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Overall really good work!! The new styling looks very nice :)
Some feedback:
- in some places, tailwindCSS uses multi-line format with backticks. Could you change this to be condensed in a single line with no backticks, to be consistent with the rest of the codebase?
- the averia font can be referenced with just 'font-averia' since its in globa.css
Overall looks really good, should just be some really quick changes. Let me know when you need a re-review!
| {/* ── NAVBAR ── */} | ||
| <nav | ||
| className={` | ||
| fixed top-0 left-0 right-0 z-50 |
There was a problem hiding this comment.
nitpick for consistency: could you put flatten the tailwindCSS to a single line, instead of using the template literal? here + other areas too
There was a problem hiding this comment.
Good catch! I have updated the className template literals to single lines to match the codebase style
| </nav> | ||
|
|
||
| {/* ── MOBILE MENU ── */} | ||
| <div |
There was a problem hiding this comment.
A few things on this menu
- bug: body scroll isn't locked while the menu is open. so the page scrolls underneath it.
- bug: the menu is anchored at
top-[88px]independent of the navbar's auto-hide. Scroll down with the menu open and the navbar slides off, leaving the menu floating with no nav above it. Either closemenuOpenwhenhiddenbecomes true, or freeze the auto-hide while the menu is open
There was a problem hiding this comment.
Both bugs should be fixed. I used useEffect which sets document.body.style.overflow to hidden when the menu is open. This should lock the body scroll. I've used a ref to track menuOpen inside the scroll handler. The navbar should not slide away while the menu is open. This should resolve the auto-hide issue.
|
Overall, I agree with all the comments and have implemented them in the new commits. Please let me know if there is anything else that needs to be highlighted or changed 👍 |
joengy
left a comment
joengy
left a comment
There was a problem hiding this comment.
Really great work on the changes, and its nice to see all the changes you made. Just one small follow up on the scroll on load behaviour, I left a comment with an pretty detailed explanation. Everything else is looking good from my side 👍
| lastY = y | ||
| } | ||
|
|
||
| handleScroll() |
There was a problem hiding this comment.
Thanks for adding this! Unfortunately I don't think it quite fixes the bug yet, let me walk through why so it makes sense.
When the page first loads, you set lastY = window.scrollY on the line just above. Then handleScroll() runs straight away, and inside it y = window.scrollY too. So y and lastY are the same number at this point.
That means:
y > lastY→ false (they're equal), so the navbar doesn't get hiddeny <= lastY→ true, so we runsetHidden(false)and the navbar stays visible
The problem is that on first load there's no "scroll direction" to detect yet (the user hasn't moved). So checking direction here can't tell us whether to hide the navbar.
The simplest fix is to just look at where the user is on the page (not which way they're moving). Something like:
setHidden(window.scrollY > 80)
at the top of the effect. That way, if someone refreshes while already scrolled down, the navbar starts hidden like it should. Then your existing scroll handler takes over from there.
Try it out by scrolling down the page and refreshing, you should see the navbar stay hidden until you scroll up. Let me know if anything's unclear!!
There was a problem hiding this comment.
Thank you so much for the feedback Joe. You are most certainly right, let me try implementing it this way 👍
|
I have tested the case where user refreshes while already scrolled down, the navbar does indeed start hidden. Great catch to identify this bug 👍. The feedback was very useful and will be continue to learn and do my best whenever possible. Let me know if there are still any changes that need to be made :) |
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Looks really good thanks Gladywn!!!
3 participants
What does this PR do?
Created a navbar half sticky, used colour scheme from global.css for button and text colours. Added the neccessary links to different pages: \home, \events, \about, \sponsors. Responsive to mobile version with hamburger feature.
Type of change
Checklist
feature/,fix/,chore/,hotfix/)