-
Notifications
You must be signed in to change notification settings - Fork 130
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: Enhance Navbar Styling and improve mobile navigation #686
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Yeah! You did it 🎉 Now, Relax 😉, Grab a drink ☕, and wait for the maintainers to check your contributions. Meanwhile, you can discuss on other issues and solve them 😀. Thank You 😃!
Meanwhile you can also discuss about the project in our Discord Server 😀
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Website/app/Header.js (2)
20-22
: Improved button styling enhances visual appeal.The updated NavLink styling with new utility classes for borders, padding, rounded corners, and hover effects significantly improves the visual appeal of the navbar buttons. This change aligns well with the PR objective.
Consider extracting these common styles into a separate CSS class or Tailwind component to improve maintainability and consistency across the application. For example:
const navLinkStyles = "border-2 px-3 py-2 text-white rounded hover:border-[#191B33] hover:bg-[#191B33] hover:scale-105 duration-500 text-sm lg:text-base"; // Usage <button onClick={() => handleNavLinkClick(to, setOpen)} className={`${navLinkStyles} ${cn}`} > {children} </button>This approach would make it easier to update the styling consistently across all NavLink components in the future.
Line range hint
27-75
: MobileNav component updated consistently with new styling.The MobileNav component has been updated to use the new NavLink styling, maintaining consistency between desktop and mobile navigation. This change aligns well with the overall navbar enhancement objective.
To improve accessibility, consider adding an
aria-label
attribute to the mobile navigation toggle button. This will help screen reader users understand the purpose of the button. For example:<div className="z-40 flex relative w-8 h-8 flex-col justify-between items-center md:hidden" onClick={() => { setOpen(!open); }} role="button" aria-label="Toggle mobile navigation" > {/* ... existing spans ... */} </div>This small addition will enhance the user experience for users relying on assistive technologies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Website/package-lock.json
is excluded by!**/package-lock.json
,!**/*.json
Website/package.json
is excluded by!**/*.json
📒 Files selected for processing (1)
- Website/app/Header.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
Website/app/Header.js (1)
Line range hint
1-191
: Overall, the navbar changes successfully enhance visual appeal.The changes made to the
Header.js
component effectively improve the aesthetic appeal of the navbar buttons, aligning well with the PR objectives. The new styling has been consistently applied to both desktop and mobile navigation components, maintaining a cohesive look across different screen sizes.Key improvements:
- Enhanced button styling with better visual feedback on hover.
- Consistent application of new styles across NavLink components.
- Preserved functionality while improving aesthetics.
The changes do not introduce any apparent issues or regressions, and the overall structure and behavior of the component remain intact.
To further improve the code:
- Consider extracting common styles into a separate CSS class or Tailwind component for better maintainability.
- Add an
aria-label
to the mobile navigation toggle button for improved accessibility.These minor enhancements will contribute to a more maintainable and accessible codebase.
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.
@yalsik86 Awesome work 👏. Just one change needed.
Please make the buttons a bit larger for mobiles.
Currently it looks like this 👇 (tested in Pixel 7 phone)
I think the scale in the hover is innecesary. When the user is seeing below the first section -> the hover background on this anchors is not different from the div background. My proposal: (two anchors normal and two on hover effect) |
As @yalsik86 has been inactive for over a month and @FabrizioJordan has created an issue to revamp the website design in #720, I'm closing this PR. @yalsik86 Feel free to open whenever you want to make any further changes. |
Fixes #682
Changes proposed
changes to navbar buttons to make the home page more aesthetically pleasing.
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers
@SaptarshiSarkar12 please review it.
Summary by CodeRabbit
New Features
Bug Fixes