Fix: keep parent menu + siblings visible on submenu pages#1
Fix: keep parent menu + siblings visible on submenu pages#1rmorse wants to merge 1 commit intoverygoodplugins:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe changes extend submenu visibility logic in content.js to recognize li elements with the 'wp-has-current-submenu' class as current items. A new showAll parameter is added to the cleanSubmenu function to force submenu items to be marked as core. The function includes logic to pre-emptively mark submenu items as core when links are missing or URL parsing fails. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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
🧪 Generate unit tests (beta)
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 |
![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
🤖 Fix all issues with AI agents
In `@content.js`:
- Around line 276-280: The early return when showAll is true prevents setting
the current highlight; update the logic in the block handling showAll (where
li.setAttribute('data-vgp-core', 'true') is called) to also set or preserve the
data-vgp-current attribute for the item that should be current (or explicitly
clear/set it based on currentItem state) before returning, or remove the early
return and let the subsequent current-item assignment code run so the current
element receives data-vgp-current while all elements still get data-vgp-core.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
content.js
🔇 Additional comments (3)
content.js (3)
238-240: LGTM!The extended
isCurrentcheck correctly identifies active submenu states by detecting thewp-has-current-submenuclass on the parentlielement. This aligns with WordPress's behavior where the parent menu item is marked with this class rather than addingcurrentto the anchor tag.
264-265: LGTM!The
showAllItemsflag correctly derives from thewp-has-current-submenuclass presence and is properly passed tocleanSubmenu(). This ensures sibling submenu items remain visible when navigating within a plugin's submenu pages, maintaining navigation context.
302-305: LGTM!The
isCurrentdetection for submenu items correctly checks multiple conditions that WordPress uses to mark current items, providing comprehensive coverage.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // If showAll is true, mark all items as core to keep them visible | ||
| if (showAll) { | ||
| li.setAttribute('data-vgp-core', 'true'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Consider setting data-vgp-current even when showAll is true.
When showAll is true, all items are marked as data-vgp-core but the actual current item won't receive the data-vgp-current attribute due to the early return. If your CSS uses data-vgp-current for highlighting the active submenu item, this could cause a styling inconsistency where the current item loses its visual emphasis.
If highlighting isn't needed in this scenario, the current implementation is fine.
Proposed fix to preserve current item highlighting
// If showAll is true, mark all items as core to keep them visible
if (showAll) {
li.setAttribute('data-vgp-core', 'true');
- return;
+ // Still check if this is the current item for styling purposes
+ const link = li.querySelector('a');
+ if (link && (link.classList.contains('current') || li.classList.contains('current'))) {
+ li.setAttribute('data-vgp-current', 'true');
+ } else {
+ li.removeAttribute('data-vgp-current');
+ }
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If showAll is true, mark all items as core to keep them visible | |
| if (showAll) { | |
| li.setAttribute('data-vgp-core', 'true'); | |
| return; | |
| } | |
| // If showAll is true, mark all items as core to keep them visible | |
| if (showAll) { | |
| li.setAttribute('data-vgp-core', 'true'); | |
| // Still check if this is the current item for styling purposes | |
| const link = li.querySelector('a'); | |
| if (link && (link.classList.contains('current') || li.classList.contains('current'))) { | |
| li.setAttribute('data-vgp-current', 'true'); | |
| } else { | |
| li.removeAttribute('data-vgp-current'); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@content.js` around lines 276 - 280, The early return when showAll is true
prevents setting the current highlight; update the logic in the block handling
showAll (where li.setAttribute('data-vgp-core', 'true') is called) to also set
or preserve the data-vgp-current attribute for the item that should be current
(or explicitly clear/set it based on currentItem state) before returning, or
remove the early return and let the subsequent current-item assignment code run
so the current element receives data-vgp-current while all elements still get
data-vgp-core.
1 participant
Problem
When viewing a plugin's submenu page, the extension hides the parent menu item (if non-core) and sibling submenu items, losing navigation context.
Cause
applyCleanUp()checkedlink.classList.contains('current')but WordPress setswp-has-current-submenuon the parentli, notcurrenton theatag. AlsocleanSubmenu()only showed items matching core slugs or havingcurrentclass.Fix
wp-has-current-submenuclass on parentlito detect active submenu stateshowAllflag tocleanSubmenu()when parent has active submenushowAllis true, mark all sibling submenu items as visible