fix: collapsed menu DOM and submenu indicators#9585
Conversation
Co-authored-by: 陈帅 <wasd2144@hotmail.com>
Co-authored-by: 陈帅 <wasd2144@hotmail.com>
Co-authored-by: 陈帅 <wasd2144@hotmail.com>
移除 collapse settled 延迟居中逻辑；收起态标题使用 flex-start 对齐 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
移除 `--pro-layout-nav-item-font-size` 的 `calc(...+ 1px)`，避免 15px Co-authored-by: 陈帅 <wasd2144@hotmail.com>
内联与侧栏弹出使用右箭头旋转，顶栏弹出使用下箭头旋转 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
- 收起且标题为纯文本时不再使用 antd Tooltip 外包层，改为原生 title - 菜单项 flex 仅作用于 item-title / Link / 标题包裹层，避免整行比图标大 - 收起态 item-title 使用 fit-content 与 iconBox 级 minWidth；nav--collapsed 不再强制 width 100% - 移除 layout index 快照测试并删除对应 snap 文件 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 Walkthrough总览本次变更修改了颜色选择器的类型定义以解除对 antd 的依赖，调整了侧边栏菜单的布局模式从混合到统一的垂直模式，更新了菜单项的样式（字重、对齐、尺寸），并将菜单模式类型从五个值缩减到两个。 变更内容
代码审查工作量评估🎯 3 (中等) | ⏱️ ~20 分钟 诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ProLayout menu components and styles, specifically addressing issues with submenu icons, collapsed state layouts, and TypeScript declaration emission. Key changes include the introduction of a flexible title rendering helper for submenus, CSS adjustments to prevent menu items from over-stretching in collapsed mode, and the replacement of snapshot tests with more targeted assertions. Feedback focuses on maintaining type safety in the ColorPicker component, preserving dynamic font size calculations in the menu styles, and ensuring that flex properties are correctly overridden to achieve the desired 'fit-content' width for menu item titles.
| }; | ||
| /** 使用宽松字段类型，避免 d.ts 生成时拉入 `@rc-component/color-picker` 导致 TS2742 */ | ||
| export type ProFormColorPickerProps = ProFormFieldItemProps< | ||
| Record<string, unknown> |
There was a problem hiding this comment.
Using Record<string, unknown> for ProFormColorPickerProps removes type safety for the component's fieldProps. While this is intended to avoid TS2742 errors during declaration emission, it's a significant trade-off for a library. Consider if a more specific local type or using import type more effectively could resolve the emission issue without losing type safety for consumers.
| [navVar.itemRadius]: '6px', | ||
| [navVar.itemGap]: '8px', | ||
| [navVar.itemFontSize]: 'calc(var(--ant-font-size) + 1px)', | ||
| [navVar.itemFontSize]: '14px', |
There was a problem hiding this comment.
Hardcoding itemFontSize to 14px is a regression from the previous dynamic calculation calc(var(--ant-font-size) + 1px). This change ignores the Ant Design global font size configuration and also changes the default value from 15px to 14px. It is recommended to maintain the use of the theme variable to ensure consistency across different themes.
| [navVar.itemFontSize]: '14px', | |
| [navVar.itemFontSize]: 'calc(var(--ant-font-size, 14px) + 1px)', |
| }, | ||
| [`${c}-item-title`]: { | ||
| width: '100%', | ||
| width: 'fit-content', |
There was a problem hiding this comment.
The PR aims to prevent the item-title from stretching to 100% width in collapsed mode. However, because the base style for item-title (defined at line 190) includes flex: 1, it will continue to grow and fill the flex container even with width: 'fit-content'. To correctly implement the desired behavior, you should also set flex: none (or flex-grow: 0) to prevent the element from stretching.
| width: 'fit-content', | |
| width: 'fit-content', | |
| flex: 'none', |
使用 grid-template-rows 0fr/1fr 与 overflow 内层，收起时 inert 防焦点 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
`item-title-collapsed` 不再固定为 itemHeight，便于图标+小字等场景 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
与常见顶栏菜单一致，侧栏内联与收起弹出仍保留指示器 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
- MenuMode 移除 inline 等；侧栏与底部链接菜单统一 mode=vertical - 类名 base-menu-inline 改为 base-menu-sider；子菜单动画 wrap 重命名 - 收起时分组标题不渲染；样式补充 menuItemRender 的 role=button 拉伸 - 更新 layout 相关测试与快照 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
- 弹出层内子菜单用 nestedPopupOpen 切换，不再覆盖根 popupOpenKey - 点击 document 关闭时忽略 portal 面板内目标 - 增加 layout=top 二三级菜单交互用例 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
useStyle 注册名加入 prefixCls，避免主菜单与 links 互相覆盖 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
一级子菜单 pointer 进入打开、离开延迟关闭；浮层 enter 取消关闭 顶栏 item/submenu-title 增加圆角与过渡 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
- 根级子菜单用 antd Popover 承载内容，替代 createPortal + 手算定位 - 浮层内嵌套子菜单同样用 Popover；切换根浮层时按 openKeys 预展开 - 样式：submenu-popup 改为相对定位；popover overlay 去重 padding - 测试：嵌套浮层用 document.body 断言三级文案 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
- 浮层内二三级改回内联展开，避免嵌套 Popover 挂 body 导致外层误关 - 根 Popover 恢复标题点击切换；移除挂到 nav 的 getPopupContainer（overflow 裁切） - submenu-popup 恢复 boxShadow Co-authored-by: 陈帅 <wasd2144@hotmail.com>
body 挂载时补全 `.baseClassName.hashId` 祖先及 horizontal/collapsed 修饰 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
叶子项、item-title、item-text、外包层与子菜单 title 统一高度与 padding Co-authored-by: 陈帅 <wasd2144@hotmail.com>
horizontal 下 item-text 用 inline-flex+iconBox 高度；子菜单 title 内层同步 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
下拉内叶子项不应命中顶栏 28px 等 horizontal 后代选择器 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
使用 cssinjs Keyframes；尊重 prefers-reduced-motion Co-authored-by: 陈帅 <wasd2144@hotmail.com>
仅作用于 submenu-popup 内，顶栏 horizontal 不受影响 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
移除底部指示条动画，保留背景色与柔和 boxShadow Co-authored-by: 陈帅 <wasd2144@hotmail.com>
Co-authored-by: 陈帅 <wasd2144@hotmail.com>
悬浮卡片不展示 Tooltip/Popover 小三角 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
- ProLayoutNavMenu 在非收起侧栏时渲染 RightOutlined/DownOutlined - 顶栏一级子菜单用向下箭头，浮层与内联用向右箭头并随展开旋转 - 调整 submenu-title-inner 子选择器避免箭头被 flex:1 拉伸 - 更新相关快照 Co-authored-by: 陈帅 <wasd2144@hotmail.com>
dosubot
Bot
added
the
size:XL
There was a problem hiding this comment.
Pull request overview
Adjusts ProLayout menu rendering to unify sider menus as vertical, improve collapsed/popup submenu DOM structure and indicators, and harden tests around nested top-menu popups.
Changes:
- Refactors menu rendering to be vertical-only in the sider, with Popover-based popup submenus and separate nested popup open state.
- Updates base menu styles for new DOM structure (submenu expand wrappers, indicator icons, collapsed behavior).
- Updates layout tests and snapshots (and replaces some snapshots with targeted assertions) to reflect new menu DOM and popup behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/layout/mobile.test.tsx | Updates selector to match new vertical submenu children class. |
| tests/layout/index.test.tsx | Replaces snapshots with assertions; adds regression tests for top-menu nested popups and hover behavior. |
| tests/layout/snapshots/pageHeaderWarp.test.tsx.snap | Updates snapshots for new vertical menu DOM structure and indicators. |
| tests/layout/snapshots/mobile.test.tsx.snap | Updates snapshots for new vertical menu DOM structure and indicators on mobile. |
| tests/layout/snapshots/index.test.tsx.snap | Removes outdated snapshots after moving tests to targeted assertions. |
| src/layout/components/SiderMenu/types.ts | Narrows MenuMode to `'vertical' |
| src/layout/components/SiderMenu/style/menu.ts | Updates CSS-in-JS styles for new DOM, animation, indicators, and registration keying. |
| src/layout/components/SiderMenu/SiderMenu.tsx | Forces sider BaseMenu mode to vertical and aligns link menu styling. |
| src/layout/components/SiderMenu/ProLayoutNavMenu.tsx | Reworks popup submenu rendering using Popover, adds nested popup open state, and updates title rendering/indicators. |
| src/layout/components/SiderMenu/BaseMenu.tsx | Improves collapsed tooltip behavior and aligns logic to vertical vs horizontal modes. |
| src/form/components/ColorPicker/index.tsx | Changes exported prop typing to avoid TS2742 during d.ts generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * 主导航渲染模式（与历史 antd Menu `mode` 命名对齐）。 | ||
| * 主导航渲染模式：顶栏 `horizontal`，侧栏 `vertical`（展开与收起均如此，收起时子菜单为弹出层）。 | ||
| */ | ||
| export type MenuMode = | ||
| | 'vertical' | ||
| | 'vertical-left' | ||
| | 'vertical-right' | ||
| | 'horizontal' | ||
| | 'inline'; | ||
| export type MenuMode = 'vertical' | 'horizontal'; |
There was a problem hiding this comment.
MenuMode is exported from src/layout/index.tsx as a public type, but this change narrows it from the historical antd-like union (inline, vertical-left/right, etc.) down to only 'vertical' | 'horizontal'. That is a breaking API change for downstream users that still pass/declare the older values. If the runtime now treats legacy values as aliases, consider keeping them in the exported union (or introducing a separate internal type and mapping legacy values to 'vertical'/'horizontal') to preserve compatibility in a patch release.
| /** 使用宽松字段类型，避免 d.ts 生成时拉入 `@rc-component/color-picker` 导致 TS2742 */ | ||
| export type ProFormColorPickerProps = ProFormFieldItemProps< | ||
| Record<string, unknown> | ||
| > & { | ||
| popoverProps?: PopoverProps; | ||
| colors?: string[]; | ||
| }; |
There was a problem hiding this comment.
This PR is described as a menu DOM/submenu indicator fix, but it also changes the exported ProFormColorPickerProps type (dropping ColorPickerProps in favor of Record<string, unknown>). Since this affects the public form API and type-safety, it should be called out explicitly in the PR description/changelog (or split into a separate PR) so reviewers and consumers understand the impact.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/layout/components/SiderMenu/BaseMenu.tsx`:
- Around line 68-74: The current branch clones props.children and injects
title/aria-label even when children is a React.Fragment, causing React warnings;
change the condition to only clone when props.children is a valid React element
AND its type is not React.Fragment (e.g. const child = props.children as
React.ReactElement; if (isValidElement(child) && child.type !== React.Fragment)
{ return cloneElement(child, { title: t, 'aria-label': t } as
React.HTMLAttributes<HTMLElement>); } ), otherwise let it fall through to the
<span> wrapper branch so DOM attributes are not passed to a Fragment.
In `@src/layout/components/SiderMenu/ProLayoutNavMenu.tsx`:
- Around line 33-37: The current hideExpandIndicator uses collapsed and mode to
hide the expand arrow, which also hides arrows for nested submenus inside popup
contexts; update the condition so the indicator is only hidden for root
collapsed triggers, e.g. change the computation of hideExpandIndicator in
ProLayoutNavMenu (where ctx, collapsed, mode, popupMode, insideSubmenuPopup are
referenced) to require not insideSubmenuPopup (and/or require popupMode ===
false) — e.g. only set hideExpandIndicator when collapsed && mode === 'vertical'
&& !insideSubmenuPopup so nested popup submenus still show their expand arrows.
- Around line 256-270: The Popover usage still uses deprecated antd v6 props
overlayClassName and styles.container; update the Popover props in
ProLayoutNavMenu.tsx to use the v6 semantic API: replace overlayClassName={...}
with classNames={{ root: '...'}} (include both
`${baseClassName}-submenu-popover-overlay` and hashId), and replace styles={{
container: { padding: 0 } }} with styles={{ body: { padding: 0 } }} so the
popupContent, node.key, and open/onOpenChange handlers (setNestedPopupOpen,
setPopupOpenKey) remain wired the same.
In `@src/layout/components/SiderMenu/style/menu.ts`:
- Around line 247-252: The new submenu expand animation on the
`${c}-submenu-expand-wrap` rule currently applies transitions unconditionally;
add a `@media (prefers-reduced-motion: reduce)` branch that disables the
`grid-template-rows` transition for `${c}-submenu-expand-wrap` and also disable
the arrow icon `transform` transition in the corresponding
submenu-arrow/indicator rule so users who prefer reduced motion don't see
animations; follow project conventions by using the same pattern as other motion
rules (or `@rc-component/motion` utility) and apply the same change to the
similar block around lines marked 281-290.
In `@tests/layout/index.test.tsx`:
- Around line 491-493: The test currently uses document.body.textContent to
assert that the nested submenu ("三级页面") is shown which can produce false
positives; instead, inside the existing waitFor block replace the global
textContent check with a query scoped to the actual popover layer (e.g., locate
the submenu popup element such as the element with class/name "submenu-popup" or
the popover/menu role) and assert the visible/open state of the node that
contains "三级页面" using web-first assertions (e.g.,
expect(foundNode).toBeVisible() or toHaveText(...)); keep using waitFor and the
same "三级页面" string to find the node rather than reading
document.body.textContent.
- Around line 571-573: The test currently uses
fireEvent.pointerEnter(topSubmenuBtn!) to simulate hover, but Ant Design's
Menu/Popover hover logic is driven by mouse events; replace the pointerEnter
call with fireEvent.mouseEnter(topSubmenuBtn!) (keeping the surrounding act(...)
and the topSubmenuBtn reference) so the hover/open behavior is triggered
reliably and matches other hover tests in tests/layout/index.test.tsx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f39eb5-9e65-4922-a712-376c3781e6f6
⛔ Files ignored due to path filters (3)
tests/layout/__snapshots__/index.test.tsx.snapis excluded by!**/*.snaptests/layout/__snapshots__/mobile.test.tsx.snapis excluded by!**/*.snaptests/layout/__snapshots__/pageHeaderWarp.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
src/form/components/ColorPicker/index.tsxsrc/layout/components/SiderMenu/BaseMenu.tsxsrc/layout/components/SiderMenu/ProLayoutNavMenu.tsxsrc/layout/components/SiderMenu/SiderMenu.tsxsrc/layout/components/SiderMenu/style/menu.tssrc/layout/components/SiderMenu/types.tstests/layout/index.test.tsxtests/layout/mobile.test.tsx
| /** 内联子菜单展开/收起：grid 0fr→1fr，无需量高 */ | ||
| [`${c}-submenu-expand-wrap`]: { | ||
| display: 'grid', | ||
| gridTemplateRows: '0fr', | ||
| transition: `grid-template-rows var(--ant-motion-duration-mid, 0.2s) cubic-bezier(0.2, 0, 0, 1)`, | ||
| }, |
There was a problem hiding this comment.
为新增展开动画补上 prefers-reduced-motion 分支。
当前 grid-template-rows 和箭头 transform 都会无条件过渡；建议在同一规则内禁用动画，避免影响开启减少动态效果的用户。
♿ 建议调整
[`${c}-submenu-expand-wrap`]: {
display: 'grid',
gridTemplateRows: '0fr',
transition: `grid-template-rows var(--ant-motion-duration-mid, 0.2s) cubic-bezier(0.2, 0, 0, 1)`,
+ '@media (prefers-reduced-motion: reduce)': {
+ transition: 'none',
+ },
},
@@
[`${c}-submenu-expand-icon`]: {
flexShrink: 0,
display: 'inline-flex',
alignItems: 'center',
justifyContent: 'center',
@@
lineHeight: 1,
transition: `transform var(--ant-motion-duration-mid, 0.2s)`,
+ '@media (prefers-reduced-motion: reduce)': {
+ transition: 'none',
+ },
svg: {
display: 'block',
},
},As per coding guidelines: Use @rc-component/motion or CSS transitions for animations, respecting prefers-reduced-motion.
Also applies to: 281-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/layout/components/SiderMenu/style/menu.ts` around lines 247 - 252, The
new submenu expand animation on the `${c}-submenu-expand-wrap` rule currently
applies transitions unconditionally; add a `@media (prefers-reduced-motion:
reduce)` branch that disables the `grid-template-rows` transition for
`${c}-submenu-expand-wrap` and also disable the arrow icon `transform`
transition in the corresponding submenu-arrow/indicator rule so users who prefer
reduced motion don't see animations; follow project conventions by using the
same pattern as other motion rules (or `@rc-component/motion` utility) and apply
the same change to the similar block around lines marked 281-290.
| act(() => { | ||
| fireEvent.pointerEnter(topSubmenuBtn!); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Testing Library fireEvent pointerEnter mouseEnter React onMouseEnter
💡 Result:
In React Testing Library, to test onMouseEnter handlers, use fireEvent.mouseEnter(element) from '@testing-library/react'. This works because RTL maps it internally to the appropriate native events (mouseOver/mouseOut) that React uses to trigger synthetic onMouseEnter. fireEvent.pointerEnter may not trigger React's onPointerEnter reliably due to React's event delegation differences; use pointerOver instead if needed, or prefer user-event. Prefer @testing-library/user-event for more realistic simulations: const user = userEvent.setup; await user.hover(element); which moves the pointer to the element, triggering hover/mouseenter events naturally. Example code: js import { render, screen } from '@testing-library/react'; import { fireEvent } from '@testing-library/react'; // or '@testing-library/dom' but prefer RTL for React import userEvent from '@testing-library/user-event'; test('tests onMouseEnter with fireEvent', => { const handleMouseEnter = jest.fn; render(
Citations:
- 1: fireEvent used with pointerEnter and pointerLeave don't fire in React testing-library/react-testing-library#783
- 2: fireEvent - mouseEnter/mouseLeave not working with addEventListener testing-library/react-testing-library#577
- 3: https://testing-library.com/docs/api-events
- 4: fix: fireEvent.mouseEnter not forwarding relatedTarget properly testing-library/react-testing-library#1429
- 5: fireEvent.mouseEnter/Leave don't trigger React component's onMouseEnter/Leave testing-library/react-testing-library#630
- 6: https://testing-library.com/docs/user-event/convenience/
- 7: https://testing-library.com/docs/user-event/convenience
🏁 Script executed:
head -n 580 tests/layout/index.test.tsx | tail -n 20Repository: ant-design/pro-components
Length of output: 592
🏁 Script executed:
head -n 600 tests/layout/index.test.tsx | tail -n 50 | cat -nRepository: ant-design/pro-components
Length of output: 1608
🏁 Script executed:
rg "fireEvent\.(mouseEnter|pointerEnter)" tests/layout/index.test.tsxRepository: ant-design/pro-components
Length of output: 114
🏁 Script executed:
rg "rc-trigger|Popover" src --type tsx --type ts -A 2 | head -50Repository: ant-design/pro-components
Length of output: 96
🏁 Script executed:
rg "rc-trigger|Popover" src -t typescript -t tsx -A 2 | head -100Repository: ant-design/pro-components
Length of output: 103
🏁 Script executed:
rg "fireEvent\.mouse|user\.hover" tests --type typescript --type tsx -lRepository: ant-design/pro-components
Length of output: 103
🏁 Script executed:
rg "rc-trigger|Popover" src -A 2 | head -100Repository: ant-design/pro-components
Length of output: 5896
🏁 Script executed:
rg "fireEvent\.(mouse|pointer|hover)" tests --glob="*.test.*" -B 2 -A 2Repository: ant-design/pro-components
Length of output: 19269
改用 mouseEnter 触发 Popover 的 hover 行为
Ant Design 菜单组件默认基于鼠标事件处理 hover 状态；pointerEnter 可能无法可靠地触发打开逻辑，导致测试行为与实际用户交互不一致。整个测试套件中其他 hover 测试也采用 fireEvent.mouseEnter()。
建议调整
act(() => {
- fireEvent.pointerEnter(topSubmenuBtn!);
+ fireEvent.mouseEnter(topSubmenuBtn!);
});📝 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.
| act(() => { | |
| fireEvent.pointerEnter(topSubmenuBtn!); | |
| }); | |
| act(() => { | |
| fireEvent.mouseEnter(topSubmenuBtn!); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/layout/index.test.tsx` around lines 571 - 573, The test currently uses
fireEvent.pointerEnter(topSubmenuBtn!) to simulate hover, but Ant Design's
Menu/Popover hover logic is driven by mouse events; replace the pointerEnter
call with fireEvent.mouseEnter(topSubmenuBtn!) (keeping the surrounding act(...)
and the topSubmenuBtn reference) so the hover/open behavior is triggered
reliably and matches other hover tests in tests/layout/index.test.tsx.
Made-with: Cursor
dosubot
Bot
added
size:M
Summary
horizontal) popup submenu: nested levels use separatenestedPopupOpenstate so clicking level-2 does not replace the rootpopupOpenKeyand unmount the panel. Outside-click handler now ignores clicks inside the portaled popup (popupPanelRef).layout top: popup shows second level and third level after clicks.Testing
pnpm run tscpnpm exec vitest run tests/layout/index.test.tsxSummary by CodeRabbit
发布说明
样式改进
功能优化