-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
fix: Navbar section updated for mobile view #320
Conversation
Someone is attempting to deploy a commit to the Sanchit Bajaj's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe recent changes aim to enhance the layout and styling of the Navbar component to improve visual presentation and user experience on both desktop and mobile views. Specific adjustments were made to elements like the hamburger menu button, theme button, login/register links, and the mobile menu. Additionally, a minor formatting change was implemented in the themeButton component. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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.
Great job, @AbhiDiva96! 🎉 Thank you for submitting your pull request. Your contribution is valuable and we appreciate your efforts to improve our project. Keep being the part of the community!
'We will promptly review your changes and offer feedback. Keep up the excellent work!
Kindly remember to check our Contributing Guidelines'
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
Out of diff range and nitpick comments (2)
src/components/core/navbar/index.tsx (2)
Line range hint
135-148
: Ensure consistent visibility of the hamburger menu button.The hamburger menu button's visibility is toggled based on the
isMenuOpen
state. However, the button is hidden whenisMenuOpen
is true, which might confuse users who want to close the menu. Consider always showing the button and changing its icon based on the state.- className={`px-2 lg:hidden dark:text-white dark:hover:text-primary text-secondary hover:text-primary transition-all duration-300 focus:outline-none ${ - isMenuOpen ? "hidden" : "closed" - }`} + className="px-2 lg:hidden dark:text-white dark:hover:text-primary text-secondary hover:text-primary transition-all duration-300 focus:outline-none"And then use the
Menu
orX
icon conditionally:{isMenuOpen ? <X size={32} /> : <Menu size={32} />}
Line range hint
211-248
: Optimize the mobile menu for better accessibility and performance.The mobile menu is currently implemented with a conditional rendering that checks
isMenuOpen
. This approach can lead to performance issues on devices with slower rendering capabilities as it involves mounting and unmounting components. Consider using CSS to hide and show the menu instead of conditional rendering to improve performance and smoothness of animations.- {isMenuOpen && ( - <div className="bg-secondary-light backdrop-blur-sm bg-opacity-60 dark:bg-primary-light dark:bg-opacity-60 h-[100vh] z-50 w-1/2 fixed inset-y-0 right-0 md:hidden transition-transform duration-300"> + <div className={`bg-secondary-light backdrop-blur-sm bg-opacity-60 dark:bg-primary-light dark:bg-opacity-60 h-[100vh] z-50 w-1/2 fixed inset-y-0 right-0 md:hidden transition-transform duration-300 ${isMenuOpen ? 'translate-x-0' : 'translate-x-full'}`}>This change uses CSS transitions for the
translate-x
property to slide the menu in and out, which is generally smoother than mounting and unmounting the component.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
package.json
is excluded by!**/*.json
yarn.lock
is excluded by!**/*.lock
,!**/*.lock
Files selected for processing (2)
- src/components/core/navbar/index.tsx (5 hunks)
- src/components/core/themeButton/index.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- src/components/core/themeButton/index.tsx
@AbhiDiva96 why this is raised and not included into your previous PR? |
I don't want to mess up all the things on that PR. I will close that PR. When you review this one. Please go with this PR |
Hey @Sanchitbajaj02 |
PR is pulled locally and in review process. It will be merged today |
thank-you so much |
src/components/core/navbar/index.tsx
Outdated
|
||
|
||
<div className="md:hidden flex justify-end"> | ||
<div className="flex justify-end"> |
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.
we don't need extra flexboxes
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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
package.json
is excluded by!**/*.json
yarn.lock
is excluded by!**/*.lock
,!**/*.lock
Files selected for processing (1)
- src/components/core/navbar/index.tsx (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/components/core/navbar/index.tsx
@Sanchitbajaj02 |
I added reviews in this pr as well as in your old PRs. Please do that to get it merged. If you're facing problems related to raising prs and git please reach out to me on discord |
@AbhiDiva96 Please acknowledge the changes that I have raised. If you have any doubt, please respond. I will close this PR in 48hrs if you didn't |
I closed the initial two PR. |
Hi @AbhiDiva96 i have told many times above that you have to change some things to get it merged. Please fix those things then I will merge. It is not just that you create something and I merge it blindly |
dear @Sanchitbajaj02 image of commit |
take latest update from master since it has some merge conflicts. Fix them and I will merge |
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 (4)
src/components/core/navbar/index.tsx (4)
Line range hint
139-148
: The hamburger menu toggle functionality is implemented correctly. Consider adding an explicittype
attribute to the button for better standards compliance.- <button onClick={() => setMenuOpen(!isMenuOpen)} className={`...`}> + <button type="button" onClick={() => setMenuOpen(!isMenuOpen)} className={`...`}>
Line range hint
211-247
: The conditional rendering of the mobile menu is correctly implemented. Simplify the className by removing unnecessary template literals.- className={`absolute right-2 top-6 dark:text-white dark:hover:text-primary focus:outline-none ${isMenuOpen ? "open" : "closed"} text-gray-600 hover:text-primary lg:hidden`} + className="absolute right-2 top-6 dark:text-white dark:hover:text-primary focus:outline-none text-gray-600 hover:text-primary lg:hidden"
Line range hint
23-23
: Specify explicit types instead ofany
for Redux state selectors to enhance type safety and maintainability.- const userAuth = useSelector((state: any) => state.auth); + const userAuth = useSelector((state: AppState) => state.auth);
Line range hint
1-11
: Consider sorting the import statements to improve code readability and maintainability.- import { useSelector, useDispatch } from "react-redux"; - import { useEffect, useState, useCallback } from "react"; - import Link from "next/link"; - import Image from "next/image"; - import { useRouter, usePathname } from "next/navigation"; + import { useCallback, useEffect, useState } from "react"; + import { useDispatch, useSelector } from "react-redux"; + import Image from "next/image"; + import Link from "next/link"; + import { useRouter, usePathname } from "next/navigation";
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/core/navbar/index.tsx (4 hunks)
Additional Context Used
Biome (9)
src/components/core/navbar/index.tsx (9)
26-26: Unexpected any. Specify a different type.
30-30: The computed expression can be simplified without the use of a string literal.
46-46: Unexpected any. Specify a different type.
172-172: Do not use template literals if interpolation and special-character handling are not needed.
250-250: Change to an optional chain.
141-146: Provide an explicit type prop for the button element.
178-181: Provide an explicit type prop for the button element.
214-219: Provide an explicit type prop for the button element.
Import statements could be sorted:
Additional comments not posted (1)
src/components/core/navbar/index.tsx (1)
136-138
: Positioning of theThemeButton
in the mobile view looks good and aligns with the PR objectives.
Related Issue
fixes: #305
Description
Improved/enhanced the navbar section for the mobile view
Screenshots
Before change
After change
Added theme button so that user can easily shift from dark to light mode or vice-versa
Summary by CodeRabbit
Summary by CodeRabbit