-
Notifications
You must be signed in to change notification settings - Fork 191
Fix Navbar Alignment And Visibility Issues #236
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Gagan202005 <gagansinghal2005@gmail.com>
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.
Pull request overview
This PR attempts to fix navbar alignment and visibility issues by making the navbar span full width and implementing background blur effects. However, the implementation introduces several structural and visual issues.
Key Changes:
- Restructured the navbar wrapper from
<nav>containing a styled<div>to a<div>wrapping a<nav>element - Removed rounded-full styling from the navbar base, changing it from a pill-shaped to rectangular design
- Added fixed height (80px) to the outer container and adjusted overflow behavior
- Modified padding and margins for both scrolled and non-scrolled states
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| components/navbar/FloatingNavbar.tsx | Restructures navbar hierarchy by swapping nav/div elements, removes rounded border styling, adds fixed height container, and adjusts padding/margins |
| components/navbar/FloatingNavbarClient.tsx | Changes overflow behavior from visible to hidden and adds negative margin adjustment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const glassNavBase = | ||
| "relative overflow-visible rounded-full transition-all duration-300 backdrop-blur-2xl"; | ||
| "relative overflow-visible transition-all duration-300 backdrop-blur-2xl"; |
Copilot
AI
Dec 30, 2025
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 removal of 'rounded-full' from the glassNavBase constant is inconsistent with the PR description which states it preserves "existing shrink behavior." The rounded navbar was part of the original design aesthetic. The removed decorative background blur elements (the two divs with rounded-full bg-gray-200 gradients) and the rounded-full class worked together to create the floating pill-shaped navbar effect. Without rounded-full, the navbar will appear as a rectangle instead of a pill shape, which is a significant visual change not mentioned in the PR description.
components/navbar/FloatingNavbar.tsx
Outdated
| <div className="absolute -top-8 -left-6 h-24 w-24 rounded-full bg-gray-200/60 blur-2xl" /> | ||
| <div className="absolute -bottom-10 -right-8 h-32 w-32 rounded-full bg-gray-200/40 blur-3xl" /> | ||
| </div> | ||
| <div className={`w-[100%] h-[80px] ${glassNavBase} ${navGlassClasses} overflow-visible ${navShadowClasses} ${navPaddingClasses}`}> |
Copilot
AI
Dec 30, 2025
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 fixed height of 80px on the outer container may cause content overflow or clipping issues. The navbar content needs to shrink and grow based on the isScrolled state, but with a fixed container height, this could lead to content being cut off or awkward spacing. Consider using min-height or a more flexible height approach that accommodates the dynamic content sizing.
| <div className={`w-[100%] h-[80px] ${glassNavBase} ${navGlassClasses} overflow-visible ${navShadowClasses} ${navPaddingClasses}`}> | |
| <div className={`w-[100%] min-h-[80px] ${glassNavBase} ${navGlassClasses} overflow-visible ${navShadowClasses} ${navPaddingClasses}`}> |
components/navbar/FloatingNavbar.tsx
Outdated
| <div className="absolute -top-8 -left-6 h-24 w-24 rounded-full bg-gray-200/60 blur-2xl" /> | ||
| <div className="absolute -bottom-10 -right-8 h-32 w-32 rounded-full bg-gray-200/40 blur-3xl" /> | ||
| </div> | ||
| <div className={`w-[100%] h-[80px] ${glassNavBase} ${navGlassClasses} overflow-visible ${navShadowClasses} ${navPaddingClasses}`}> |
Copilot
AI
Dec 30, 2025
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.
There are conflicting overflow declarations in the same className string: both 'overflow-visible' and the overflow behavior implied by the container. The outer div has 'overflow-visible' while the inner FloatingNavbarClient div has 'overflow-hidden'. This creates confusion and potential rendering issues. The outer container should be reviewed to ensure the overflow behavior matches the intended design, especially since dropdowns and other floating elements may need to escape the container bounds.
| <div className={`w-[100%] h-[80px] ${glassNavBase} ${navGlassClasses} overflow-visible ${navShadowClasses} ${navPaddingClasses}`}> | |
| <div className={`w-[100%] h-[80px] ${glassNavBase} ${navGlassClasses} ${navShadowClasses} ${navPaddingClasses}`}> |
| ? "px-4 mb-4 pt-2 py-1.5 md:px-4 md:py-2 lg:px-5 lg:py-2.5" | ||
| : "px-5 py-2 mb-4 pt-2 md:px-6 md:py-3 lg:px-8 lg:py-4"; |
Copilot
AI
Dec 30, 2025
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.
Adding both mb-4 and pt-2 to both scrolled and non-scrolled states creates inconsistent spacing behavior. The original design had different padding for scrolled vs. non-scrolled states to create a visual size change effect. By adding these same values to both states, the visual differentiation between states may be reduced. Additionally, adding mb-4 (margin-bottom) to the navbar itself could affect layout of elements below it unexpectedly.
| ? "px-4 mb-4 pt-2 py-1.5 md:px-4 md:py-2 lg:px-5 lg:py-2.5" | |
| : "px-5 py-2 mb-4 pt-2 md:px-6 md:py-3 lg:px-8 lg:py-4"; | |
| ? "px-4 pt-2 py-1.5 md:px-4 md:py-2 lg:px-5 lg:py-2.5" | |
| : "px-5 py-2 md:px-6 md:py-3 lg:px-8 lg:py-4"; |
|
|
||
| return ( | ||
| <div className="flex items-center justify-between overflow-visible"> | ||
| <div className="flex h-full items-center overflow-hidden justify-between mt-[-6px]"> |
Copilot
AI
Dec 30, 2025
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 addition of h-full (height: 100%) on the inner flex container creates a dependency on the parent having a defined height. While the parent now has h-[80px], this coupling makes the component less flexible and could cause layout issues if the parent height changes or isn't properly set. Consider whether h-full is necessary here, or if the container should have its own explicit height specification.
| <div className="flex h-full items-center overflow-hidden justify-between mt-[-6px]"> | |
| <div className="flex items-center overflow-hidden justify-between mt-[-6px]"> |
| @@ -127,7 +127,7 @@ export default function FloatingNavbarClient({ techLatest = [], communityLatest | |||
| }, []); | |||
|
|
|||
| return ( | |||
Copilot
AI
Dec 30, 2025
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 magic number mt-[-6px] (negative 6px margin) is used without explanation. Magic numbers make code harder to maintain and understand. If this negative margin is necessary for alignment, it should be documented with a comment explaining why, or replaced with a more semantic approach to achieve the desired positioning.
| return ( | |
| return ( | |
| {/* NOTE: mt-[-6px] nudges the navbar content to visually center-align with the header/logo height. */} |
|
|
||
| return ( | ||
| <div className="flex items-center justify-between overflow-visible"> | ||
| <div className="flex h-full items-center overflow-hidden justify-between mt-[-6px]"> |
Copilot
AI
Dec 30, 2025
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.
Changing from 'overflow-visible' to 'overflow-hidden' on the inner navbar content container could clip dropdown menus and other floating UI elements that need to extend beyond the navbar bounds. The FloatingNavbarClient component contains dropdown menus for Technology and Community sections that appear on hover. With overflow-hidden, these dropdowns may be cut off or not display properly.
| <div className="flex h-full items-center overflow-hidden justify-between mt-[-6px]"> | |
| <div className="flex h-full items-center overflow-visible justify-between mt-[-6px]"> |
components/navbar/FloatingNavbar.tsx
Outdated
| <div className={`w-[100%] h-[80px] ${glassNavBase} ${navGlassClasses} overflow-visible ${navShadowClasses} ${navPaddingClasses}`}> | ||
| <nav className={`${navPositionClasses} transition-all duration-300 ${navWidthClasses}`}> | ||
| <FloatingNavbarClient isScrolled={isScrolled} /> | ||
| </div> | ||
| </nav> | ||
| </nav> | ||
| </div> |
Copilot
AI
Dec 30, 2025
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 outer div with w-[100%] combined with the inner nav's positioning classes creates a structural issue. When the navbar is in fixed positioning mode (non-blog pages), the nav element has 'fixed top-6 left-1/2 -translate-x-1/2' which positions it absolutely relative to the viewport. However, the outer div with w-[100%] and fixed height will span full width but won't contain the fixed nav properly. This breaks the intended centered, floating navbar effect. The outer container should not have positioning/sizing when the inner nav is in fixed mode, or the positioning logic needs to be reconsidered.
components/navbar/FloatingNavbar.tsx
Outdated
| <div className={`w-[100%] h-[80px] ${glassNavBase} ${navGlassClasses} overflow-visible ${navShadowClasses} ${navPaddingClasses}`}> | ||
| <nav className={`${navPositionClasses} transition-all duration-300 ${navWidthClasses}`}> | ||
| <FloatingNavbarClient isScrolled={isScrolled} /> | ||
| </div> | ||
| </nav> | ||
| </nav> | ||
| </div> |
Copilot
AI
Dec 30, 2025
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 structural change from a nav element containing a styled div to a div wrapping a nav element is semantically incorrect. The nav element should be the semantic container for navigation elements, not nested inside a presentational div. This change degrades HTML semantics and accessibility. The original structure (nav > div > FloatingNavbarClient) was more appropriate for a navigation component.
amaan-bhati
left a comment
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.
Hey @Gagan202005 Thanks for raising this pr, before i review it on local, can you please take a look at the copilot review?
Also, for the navbar width, the shrink on scroll is still supposed to be there, by default it is supposed to be the full width, but on scroll, the shrink is supposed to act the same way as it did before.
Signed-off-by: Gagan202005 <gagansinghal2005@gmail.com>
Signed-off-by: Gagan202005 <gagansinghal2005@gmail.com>
|
Hi @amaan-bhati |
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.
Hey @Gagan202005 Thanks for following up, i tried running it on local, but there are a few errors on the terminal, for the first error, you can simply try to install framer-motion properly using npm i framer-motion, and after that i still got a few errors on the terminial which i think you should take a look at.
In terms of the ui, there is still one improvment/attention to detail,
- When the navbar is getting shrinked, the navbar is not supposed to have a white backgroun on top of the header, infact it is supposed to have no background at all, it would have a transparent bg, similar to what it is right now on the blog website.
- When shrinking, the nav animation needs to be smoother, it is supposed to get smaller in height as well, for reference you can take a look at the current shrinking behvaious of the navbar.
Signed-off-by: Gagan202005 <gagansinghal2005@gmail.com>
|
Hi @amaan-bhati , Please review it and let me know how you’d like me to proceed. |
|
hi @amaan-bhati |
amaan-bhati
left a comment
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.
Hey @Gagan202005 Thanks for folloowing up on the review, i tried reviewing it again on my local but seems like the issue is not resolved yet, the shrinking of the navbar needs to be subtle and the animation needs to smooth and not so fast, there needs to be enough space at the top as it is in the current navbar after shrink, also the navbar width after the shrink needs to match the width of the other sections as well.
Signed-off-by: Gagan202005 <gagansinghal2005@gmail.com>
Signed-off-by: Gagan202005 <gagansinghal2005@gmail.com>
|
Hi @amaan-bhati Thanks a lot for the detailed review and suggestions — they were really helpful. Screen.Recording.2026-01-09.at.3.29.52.AM.movCould you please take another look and let me know if everything looks good now? Thanks again! |
|
Hi @amaan-bhati |
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.
Hey @Gagan202005 Thanks for following up on the previous review, I tried reviewing the changes on my local and i was able to replicate the same ui as shared. The ui looks good to me, but before we merge this PR, i think it would be better if we get the copilot review resolved as well, it seems like you have used negative amrgin at places, which is not really a great practice to follow.
|
hey @amaan-bhati i try to remove negative margin already but i find that it is very required for the right postioning Thanks ! |
|
Hey @amaan-bhati |


Related Tickets & Documents
Fixes: keploy/keploy#3428
Fixes: keploy/keploy#3429
Description
Updated navbar styling to span 100% width across all screen sizes while preserving the existing shrink behavior.
Implemented an efficient background blur to subtly hide underlying content and improve visual clarity.
Cleaned up background-related CSS to ensure a consistent, smooth, and responsive navbar experience.
Type of Change
Testing
Demo
Checklist