Ensure sidebar menu highlights correctly on React-based navigation#3093
Ensure sidebar menu highlights correctly on React-based navigation#3093
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.
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.
|
@MdAsifHossainNadim @mrabbani Needs dev review |
|
Note: worked initially, approved if dev review done. Test withnin translated menu too |
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.