Show menu icon on collapsed modeand add action for toogle menu#55
Open
Show menu icon on collapsed modeand add action for toogle menu#55
Conversation
📝 WalkthroughWalkthroughAdds WordPress hook-based toggling (namespaced) to Layout, exports Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Header as LayoutHeader
participant Hooks as WordPressHooks
participant Context as LayoutContext/Provider
participant Sidebar as LayoutSidebar
participant Menu as LayoutMenu
User->>Header: Click toggle button
Header->>Hooks: doAction("{namespace}_layout_toggle")
Hooks->>Sidebar: addAction listener triggers
Sidebar->>Sidebar: toggle() -> update sidebarOpen state
Sidebar->>Context: update provider value (sidebarOpen)
Context->>Menu: re-render with sidebarOpen
Menu->>Menu: compute collapsed = !sidebarOpen
alt collapsed
Menu->>Menu: render icon-only items
Menu->>Menu: show flyout submenus on hover/focus
else expanded
Menu->>Menu: render full menu tree
end
Menu->>User: UI updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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
🧪 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: 3
🧹 Nitpick comments (3)
src/components/ui/layout-menu.tsx (2)
817-825: Nested children in flyout render as a flat list without indentation.
FlyoutMenuItemrecursively rendersFlyoutMenuItemListfor deeply nested items, but all levels share the same padding (px-3). For menus deeper than 2 levels, there's no visual hierarchy to distinguish nesting depth. If deep nesting is expected, consider passing adepthprop and adding left padding, or limiting flyout rendering to one level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/layout-menu.tsx` around lines 817 - 825, FlyoutMenuItem renders FlyoutMenuItemList recursively but uses the same padding (px-3) for every level, causing deeply nested items to appear flat; modify the recursive rendering in FlyoutMenuItem (and FlyoutMenuItemList props) to accept a depth (or level) prop that increments on each recursion and apply left padding/margin proportional to depth (e.g., add className like `pl-${depth * N}` or inline style) so deeper items are indented, or alternatively stop recursion after one level and render deeper children in a secondary flyout; update the FlyoutMenuItem signature and the FlyoutMenuItemList prop usage (activeItemClassName/menuItemClassName remain the same) to pass and use the new depth value.
669-721: DuplicatedComp/compPropspattern across three components.
CollapsedMenuItemIcon,FlyoutMenuItem, andLayoutMenuItemRowall repeat the same polymorphic element logic (item.href ? "a" : "button"+ prop switching). Consider extracting a shared helper or a small polymorphic wrapper to reduce duplication. Not urgent, but worth tracking for a follow-up.Also applies to: 766-828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/layout-menu.tsx` around lines 669 - 721, Extract the repeated polymorphic element logic (item.href ? "a" : "button" and the corresponding compProps) used in CollapsedMenuItemIcon, FlyoutMenuItem, and LayoutMenuItemRow into a small shared helper or wrapper (e.g., a PolymorphicLinkButton or getPolymorphicProps function) that returns the component tag and merged props; update each component (CollapsedMenuItemIcon, FlyoutMenuItem, LayoutMenuItemRow) to call the helper and spread its returned props while preserving existing attributes (role, tabIndex, aria-label, aria-current, data-active, className, disabled, and onClick behavior) so behavior and accessibility remain identical but duplication is removed.src/components/ui/Layout.stories.tsx (1)
466-498: Nesting<header>inside<main>— valid but may confuse assistive tech.In
HeaderOnContent,LayoutHeader(which renders a<header>) is placed insideLayoutMain(which renders a<main>). While HTML5 allows this, screen readers may announce the header landmark unexpectedly within the main content region. Consider adding a note in the story docs or using a<div>wrapper with the header styling instead if this pattern is intended for production use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Layout.stories.tsx` around lines 466 - 498, The story HeaderOnContent currently nests LayoutHeader (renders a <header>) inside LayoutMain (renders a <main>), which can confuse assistive tech; either move LayoutHeader out of LayoutMain (place it as a sibling in the LayoutBody or above LayoutMain) or replace LayoutHeader with a semantic-neutral wrapper (e.g., use a div-styled header component) for this story, and add a brief docnote in the story comments explaining the accessibility implication when using LayoutHeader inside LayoutMain so consumers know this is only for visual/demo purposes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/layout-menu.tsx`:
- Around line 580-606: The flyout is only toggled by hover classes so keyboard
users can't open it; update the visibility classes on the flyout container (the
element rendered when hasChildren is true) to include the focus-within variants
alongside the existing hover variants—e.g. add
group-focus-within/flyout:pointer-events-auto, group-focus-within/flyout:visible
and group-focus-within/flyout:opacity-100 to the cn(...) string so the
FlyoutMenuItemList and its parent label (item.label) become reachable when a
descendant receives focus.
In `@src/components/ui/layout.tsx`:
- Line 14: The project imports addAction and removeAction from
"@wordpress/hooks" in src/components/ui/layout.tsx but that package is not
declared in package.json; update package.json to add "@wordpress/hooks" under
dependencies (or appropriate peerDependencies) with a compatible version, run
npm/yarn install to update lockfile, and ensure package.json's version spec is
consistent with the project's compatibility policy so the import in layout.tsx
resolves at runtime.
- Around line 118-130: The effect is re-registering on every render and
toggleSideBar is recreated each render; wrap toggleSideBar in useCallback (so it
is stable) and add a dependency array to the useEffect that includes namespace
and the stable toggleSideBar, and update the React import to include
useCallback; ensure addAction and removeAction are called only once per relevant
dependency change (e.g., useEffect(() => { addAction(hookName, namespace,
toggleSideBar); return () => removeAction(hookName, namespace); }, [namespace,
toggleSideBar])).
---
Nitpick comments:
In `@src/components/ui/layout-menu.tsx`:
- Around line 817-825: FlyoutMenuItem renders FlyoutMenuItemList recursively but
uses the same padding (px-3) for every level, causing deeply nested items to
appear flat; modify the recursive rendering in FlyoutMenuItem (and
FlyoutMenuItemList props) to accept a depth (or level) prop that increments on
each recursion and apply left padding/margin proportional to depth (e.g., add
className like `pl-${depth * N}` or inline style) so deeper items are indented,
or alternatively stop recursion after one level and render deeper children in a
secondary flyout; update the FlyoutMenuItem signature and the FlyoutMenuItemList
prop usage (activeItemClassName/menuItemClassName remain the same) to pass and
use the new depth value.
- Around line 669-721: Extract the repeated polymorphic element logic (item.href
? "a" : "button" and the corresponding compProps) used in CollapsedMenuItemIcon,
FlyoutMenuItem, and LayoutMenuItemRow into a small shared helper or wrapper
(e.g., a PolymorphicLinkButton or getPolymorphicProps function) that returns the
component tag and merged props; update each component (CollapsedMenuItemIcon,
FlyoutMenuItem, LayoutMenuItemRow) to call the helper and spread its returned
props while preserving existing attributes (role, tabIndex, aria-label,
aria-current, data-active, className, disabled, and onClick behavior) so
behavior and accessibility remain identical but duplication is removed.
In `@src/components/ui/Layout.stories.tsx`:
- Around line 466-498: The story HeaderOnContent currently nests LayoutHeader
(renders a <header>) inside LayoutMain (renders a <main>), which can confuse
assistive tech; either move LayoutHeader out of LayoutMain (place it as a
sibling in the LayoutBody or above LayoutMain) or replace LayoutHeader with a
semantic-neutral wrapper (e.g., use a div-styled header component) for this
story, and add a brief docnote in the story comments explaining the
accessibility implication when using LayoutHeader inside LayoutMain so consumers
know this is only for visual/demo purposes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ui/Layout.stories.tsxsrc/components/ui/index.tssrc/components/ui/layout-menu.tsxsrc/components/ui/layout.tsx
src/components/ui/layout.tsx
Outdated
| } from "react"; | ||
| import { Button } from "./button"; | ||
|
|
||
| import { addAction, removeAction } from "@wordpress/hooks"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if package.json exists and display its dependencies sections
echo "=== Checking package.json for `@wordpress/hooks` ==="
if [ -f "package.json" ]; then
cat package.json | jq '.dependencies, .peerDependencies, .devDependencies' 2>/dev/null | grep -i wordpress || echo "Not found in standard dependency fields"
else
echo "package.json not found at root"
fi
echo ""
echo "=== Display entire dependencies section for context ==="
if [ -f "package.json" ]; then
cat package.json | jq '.dependencies' 2>/dev/null | head -20
fi
echo ""
echo "=== Verify src/components/ui/layout.tsx exists and show import ==="
if [ -f "src/components/ui/layout.tsx" ]; then
head -20 src/components/ui/layout.tsx
else
echo "File not found at src/components/ui/layout.tsx"
fiRepository: getdokan/plugin-ui
Length of output: 1242
Add @wordpress/hooks to dependencies in package.json.
The import statement at line 14 uses @wordpress/hooks, but this package is not declared in dependencies or peerDependencies. This will cause a runtime error for consumers of the library. Add @wordpress/hooks to package.json with an appropriate version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/layout.tsx` at line 14, The project imports addAction and
removeAction from "@wordpress/hooks" in src/components/ui/layout.tsx but that
package is not declared in package.json; update package.json to add
"@wordpress/hooks" under dependencies (or appropriate peerDependencies) with a
compatible version, run npm/yarn install to update lockfile, and ensure
package.json's version spec is consistent with the project's compatibility
policy so the import in layout.tsx resolves at runtime.
…uality issues - Add dependency array to sidebar toggle useEffect to prevent re-registering on every render - Wrap toggleSideBar in useCallback for stable reference - Add focus-within support to flyout menus for keyboard accessibility - Replace try/catch useLayoutSafe with proper useLayoutOptional hook - Fix minor formatting (extra space, missing space, double blank line) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/layout.tsx (1)
248-257:⚠️ Potential issue | 🟠 MajorUse breakpoint-aware overflow classes in collapsed mode.
Line [298] and Line [320] hardcode
lg:overflow-visible, but the component supportssidebarBreakpoint="md" | "lg" | "xl". Formd/xl, collapsed flyouts can be clipped at desktop sizes.💡 Proposed fix
const desktopTransition = bp === "lg" ? "lg:transition-[width] lg:duration-200" : bp === "md" ? "md:transition-[width] md:duration-200" : "xl:transition-[width] xl:duration-200"; + const desktopOverflowVisible = + bp === "lg" + ? "lg:overflow-visible" + : bp === "md" + ? "md:overflow-visible" + : "xl:overflow-visible"; @@ - !sidebarOpen && "lg:overflow-visible", + !sidebarOpen && desktopOverflowVisible, breakpointClass, className, )} @@ sidebarOpen ? "overflow-hidden" - : "max-lg:hidden lg:overflow-visible", + : cn("max-lg:hidden", desktopOverflowVisible), )} >Also applies to: 298-300, 315-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/layout.tsx` around lines 248 - 257, The collapsed flyout overflow classes are hardcoded to "lg:overflow-visible" causing clipping when sidebarBreakpoint is "md" or "xl"; update the layout to compute a breakpoint-aware overflow class (using sidebarBreakpoint or bp) similar to breakpointClass/collapseWhenClosed (e.g., derive overflowClass as `${bp}:overflow-visible` or via a conditional mapping) and replace all occurrences of the hardcoded "lg:overflow-visible" (lines near where collapseWhenClosed, breakpointClass, and isLeft are used) so collapsed flyouts use the correct md|lg|xl overflow utility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/layout-menu.tsx`:
- Around line 670-680: The dynamic element construction using Comp/compProps (in
CollapsedMenuItemIcon and FlyoutMenuItem) currently passes
disabled={item.disabled} onto the rendered <Comp>, which is problematic when
Comp is an "a" (anchors ignore disabled); remove the explicit disabled prop from
the rendered element and rely on compProps to include disabled only for the
button branch (keep href/onClick for anchor branch and type/disabled/handleClick
for button branch), ensuring click behavior uses onItemClick/handleClick as
already defined.
---
Outside diff comments:
In `@src/components/ui/layout.tsx`:
- Around line 248-257: The collapsed flyout overflow classes are hardcoded to
"lg:overflow-visible" causing clipping when sidebarBreakpoint is "md" or "xl";
update the layout to compute a breakpoint-aware overflow class (using
sidebarBreakpoint or bp) similar to breakpointClass/collapseWhenClosed (e.g.,
derive overflowClass as `${bp}:overflow-visible` or via a conditional mapping)
and replace all occurrences of the hardcoded "lg:overflow-visible" (lines near
where collapseWhenClosed, breakpointClass, and isLeft are used) so collapsed
flyouts use the correct md|lg|xl overflow utility.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/index.tssrc/components/ui/layout-menu.tsxsrc/components/ui/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/index.ts
Comment on lines
+670
to
+680
| const Comp = item.href ? "a" : "button"; | ||
| const compProps = | ||
| item.href | ||
| ? { | ||
| href: item.href, | ||
| onClick: () => onItemClick?.(item), | ||
| } | ||
| : { | ||
| type: "button" as const, | ||
| onClick: handleClick, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/components/ui/layout-menu.tsxRepository: getdokan/plugin-ui
Length of output: 99
🏁 Script executed:
# Read the file to examine the code at specified lines
cat -n src/components/ui/layout-menu.tsx | sed -n '665,685p'Repository: getdokan/plugin-ui
Length of output: 646
🏁 Script executed:
# Check lines around 698-700
cat -n src/components/ui/layout-menu.tsx | sed -n '693,705p'Repository: getdokan/plugin-ui
Length of output: 619
🏁 Script executed:
# Check lines around 770-780
cat -n src/components/ui/layout-menu.tsx | sed -n '765,785p'Repository: getdokan/plugin-ui
Length of output: 652
🏁 Script executed:
# Check lines around 798-800
cat -n src/components/ui/layout-menu.tsx | sed -n '793,805p'Repository: getdokan/plugin-ui
Length of output: 574
Disabled items with href are still interactive because anchors ignore the disabled attribute.
Lines 698 and 798 pass disabled={item.disabled} to a dynamic Comp that can be <a>. HTML anchor elements don't support disabled, so items with both href and disabled: true remain focusable and clickable.
Apply this fix to both locations (CollapsedMenuItemIcon at lines 670–700 and FlyoutMenuItem at lines 770–800):
Proposed fix
- const Comp = item.href ? "a" : "button";
+ const isLink = Boolean(item.href) && !item.disabled;
+ const Comp = isLink ? "a" : "button";
const compProps =
- item.href
+ isLink
? {
href: item.href,
onClick: () => onItemClick?.(item),
}
: {
type: "button" as const,
onClick: handleClick,
+ disabled: item.disabled,
};Remove disabled={item.disabled} from the rendered <Comp> element since the button branch now includes it in compProps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/layout-menu.tsx` around lines 670 - 680, The dynamic
element construction using Comp/compProps (in CollapsedMenuItemIcon and
FlyoutMenuItem) currently passes disabled={item.disabled} onto the rendered
<Comp>, which is problematic when Comp is an "a" (anchors ignore disabled);
remove the explicit disabled prop from the rendered element and rely on
compProps to include disabled only for the button branch (keep href/onClick for
anchor branch and type/disabled/handleClick for button branch), ensuring click
behavior uses onItemClick/handleClick as already defined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation