Fix hydration error in Header.js #674
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the navigation functionality within the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 😀
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Website/app/Header.js (2)
49-51: Remove unnecessarysetOpenprop fromNavLinkin desktop navigationIn the desktop navigation, the
NavLinkcomponents are being passedsetOpen={setOpen}, but thesetOpenprop is primarily used for controlling the mobile navigation state. In the desktop layout, the navigation menu is always visible, and there's no need to close it.Consider removing
setOpen={setOpen}fromNavLinkin the desktop navigation to simplify the code.Apply this diff:
// In the desktop navigation at lines 154-164 <div className="hidden md:flex mr-20 "> - <NavLink to="/#" cn="mx-4 flex" setOpen={setOpen}> + <NavLink to="/#" cn="mx-4 flex"> HOME </NavLink> - <NavLink to="/about" cn="mx-4 flex" setOpen={setOpen}> + <NavLink to="/about" cn="mx-4 flex"> ABOUT </NavLink> - <NavLink to="/download" cn="mx-4 flex" setOpen={setOpen}> + <NavLink to="/download" cn="mx-4 flex"> DOWNLOAD </NavLink> - <NavLink to="/contact" cn="mx-4 flex" setOpen={setOpen}> + <NavLink to="/contact" cn="mx-4 flex"> CONTACT </NavLink> </div>Also applies to: 56-58, 63-65, 154-164
Line range hint
27-73: Simplify navigation logic by removing unnecessary functionsWith the adoption of the
<Link>component and client-side routing, thehandleNavLinkClickfunction and passingsetOpento everyNavLinkmay no longer be necessary.Consider refactoring
MobileNavandNavLinkto streamline your navigation:
- Remove the
handleNavLinkClickfunction.- Pass
setOpendirectly toonClickhandlers where needed.- Ensure
setOpen(false)is called after navigation to close the mobile menu.Apply this diff as a starting point:
-function handleNavLinkClick(to, setOpen) { - if (window.location.pathname === to) { - setOpen(false); - } else { - window.location.href = to; - } -} - function NavLink({ to, children, cn }) { return ( - <button - onClick={() => handleNavLinkClick(to, setOpen)} + <Link + href={to} + onClick={() => setOpen(false)} className={`text-gray-900 hover:text-black ${cn}`} > {children} - </button> + </Link> ); }Ensure that
setOpen(false)is called appropriately to close the mobile navigation after a link is clicked.
|
@drippypop Awesome work 👏. |
|
@SaptarshiSarkar12 fixing it right now |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
Website/app/Header.js (3)
156-166: LGTM: Header component changes with minor suggestionThe modifications to the
Headercomponent, including passingsetOpentoMobileNavand eachNavLink, are consistent with the changes made to other components. This implementation effectively addresses the issue of the navigation bar not closing when a link is clicked.For consistency, consider updating the "HOME" NavLink to use a lowercase path:
- <NavLink to="/#" cn="mx-4 flex" setOpen={setOpen}> + <NavLink to="/" cn="mx-4 flex" setOpen={setOpen}>This change aligns with the path format used in other NavLink components and removes the unnecessary hash.
Line range hint
137-139: Use functional update forsetOpenThe current implementation directly toggles the
openstate. For consistency and to avoid potential issues with state updates, it's recommended to use the functional form ofsetOpen.Update the click handler as follows:
- onClick={() => { - setOpen(!open); - }} + onClick={() => setOpen(prevOpen => !prevOpen)}This change ensures that the state update is based on the previous state, which is a more reliable approach, especially when dealing with asynchronous state updates.
Line range hint
1-187: Overall good implementation with suggestions for improvementThe changes in this file effectively address the hydration error and the issue of the navigation bar not closing when clicking on the current page. The implementation of
handleNavLinkClickand the modifications toNavLink,MobileNav, andHeadercomponents work together to solve the described problems.To further improve the code quality and consistency:
- Consider using a linter (e.g., ESLint) with a consistent configuration to standardize code style, including quotation marks usage.
- Implement the suggestions provided in the previous comments, particularly:
- Updating the router import for the Next.js App Router
- Refactoring
handleNavLinkClickfor better Next.js integration- Preserving the
Linkcomponent inNavLinkfor SEO and accessibility- Using a functional update for
setOpenin the hamburger button click handlerThese changes will enhance the overall quality, maintainability, and performance of the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Website/app/Header.js (4 hunks)
🔇 Additional comments (1)
Website/app/Header.js (1)
29-29: LGTM: MobileNav changesThe modifications to the
MobileNavcomponent, including passing thesetOpenprop to eachNavLink, effectively address the issue of the navigation bar not closing when a link is clicked. This implementation aligns well with the PR objectives.Also applies to: 51-51, 58-58, 65-65
|
@drippypop Please fix the reviews and errors by GitHub Advanced Security bot and Coderabbit AI bot. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
Website/app/Header.js (2)
155-165: LGTM with suggestion: NavLink updates in main navigationThe addition of the
setOpenprop to theNavLinkcomponents in the main navigation is consistent with the earlier changes and improves functionality.However, consider optimizing this for desktop views:
- You could conditionally pass the
setOpenprop based on the viewport size:const isMobile = useMediaQuery('(max-width: 768px)'); // In the JSX <NavLink to="/#" cn="mx-4 flex" setOpen={isMobile ? setOpen : undefined} > HOME </NavLink> // Repeat for other NavLinksThis would prevent unnecessary state updates on desktop views where the mobile navigation isn't used.
13-13: Ensure router is defined before accessingWhile the static analysis tool flags a potential issue with
router.push(to), this is likely a false positive given the context. However, it's always good to add a safety check.Consider adding a null check before using the router:
if (router && typeof router.push === 'function') { router.push(to); } else { // Fallback behavior, e.g., using window.location window.location.href = to; }This will make the code more robust and prevent potential runtime errors if
routeris unexpectedly undefined.🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 13-13: Property access on null or undefined
The base expression of this property access is always undefined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Website/app/Header.js (4 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
Website/app/Header.js
[notice] 7-7: Unused variable, import, function or class
Unused import useRouter.
[failure] 13-13: Property access on null or undefined
The base expression of this property access is always undefined.
🔇 Additional comments (2)
Website/app/Header.js (2)
28-28: LGTM: MobileNav prop updateThe addition of the
setOpenprop to theMobileNavcomponent is a good change. It allows the component to control the mobile navigation state, which is consistent with the modifications made to theNavLinkcomponent.
50-50: LGTM: NavLink updates in MobileNavThe addition of the
setOpenprop to theNavLinkcomponents withinMobileNavis correct and consistent with the earlier changes. This modification ensures that the mobile navigation closes when a link is clicked, improving the user experience.Also applies to: 57-57, 64-64
|
gosh.. what's going on lol |
|
@SaptarshiSarkar12 FINALLY fixed it! |
There was a problem hiding this comment.
@drippypop The fix no longer works 😞. Please check and make the necessary changes. Probably router doesn't work as expected.
|
@drippypop As the recent fixes don't work as expected, you can revert to commit d9a158a |
|
@SaptarshiSarkar12 sorry about that! I'll make sure to revert to that |
|
@drippypop Great 😃👍 |
There was a problem hiding this comment.
@drippypop Looks good to merge 👍.
Thanks for contributing 🚀 🚀.
You may join our Discord server - https://discord.gg/DeT4jXPfkG to get updates about the project.
SaptarshiSarkar12
added
the
hacktoberfest-accepted
Description:
This PR addresses the hydration error caused by text content mismatch in the server-rendered HTML in the
Header.jsfile. The issue was due to nested<button>elements. The following changes have been made to resolve this error:<button>elements inside theNavLinkcomponent.NavLinkcomponent to use theLinkcomponent properly.setOpenfunction is called appropriately to handle toggling the mobile navigation.Changes:
Header.jsto remove nested<button>elements.NavLinkcomponent.File(s) Modified:
Website/app/Header.jsBefore:
After:
Testing:
Issue Fixed:
Reviewer:
Summary by CodeRabbit
New Features
Bug Fixes