-
Notifications
You must be signed in to change notification settings - Fork 214
Ensure sidebar menu highlights correctly on React-based navigation #3093
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: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe Sidebar component's active state detection was refactored to use new helper functions Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vendor-dashboard/layout/components/Sidebar.tsx (1)
493-497: Inconsistent active state check for inline submenu items.The
isSubActivecheck still uses the oldcurrentUrl.startsWith(subitem.url)logic, whilehasActiveChilddetection at line 335 uses the newisUrlActive()helper. This inconsistency can cause the parent menu to correctly highlight (becausehasActiveChildusesisUrlActive), but the actual submenu item may not highlight correctly (becauseisSubActiveusesstartsWith), or vice versa—particularly for hash-based or query-param URLs.🔧 Proposed fix
const isSubActive = subitem?.url && - currentUrl.startsWith( - subitem.url - ); + isUrlActive( + subitem.url + );
🧹 Nitpick comments (1)
src/vendor-dashboard/layout/components/Sidebar.tsx (1)
181-202: Consider usingisUrlActivefor auto-expand logic consistency.The initial expansion logic at line 191 still uses
currentUrl.startsWith(sub.url). For consistent behavior with hash-based and query-param URLs, consider updating this to use the sameisUrlActivelogic. Note that line 182 declares a localcurrentUrlthat shadows the state variable, so if you switch toisUrlActive, ensure it references the correct URL value.♻️ Suggested refactor
useEffect( () => { - const currentUrl = window.location?.href || ''; const initial: Record< string, boolean > = {}; + const locationHref = window.location?.href || ''; Object.entries( ( sidebarNav as any ) || {} ).forEach( ( [ key, item ]: any ) => { let hasActiveChild = false; if ( item?.submenu ) { Object.values( item.submenu ).forEach( ( sub: any ) => { - if ( sub?.url && currentUrl.startsWith( sub.url ) ) { + if ( sub?.url && isUrlActive( sub.url ) ) { hasActiveChild = true; } } ); } initial[ key ] = hasActiveChild; } ); setExpanded( initial ); - }, [ sidebarNav ] ); + }, [ sidebarNav, currentUrl ] );Note: Adding
currentUrlto the dependency array ensures the expanded state updates when the URL changes via React navigation.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.