fix: try to fix createPortal close case #492
Conversation
Walkthrough此次更改引入了多个新文件和功能。首先，创建了一个新的文档文件 Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #492 +/- ##
=======================================
Coverage 97.72% 97.73%
=======================================
Files 13 13
Lines 791 794 +3
Branches 238 239 +1
=======================================
+ Hits 773 776 +3
Misses 18 18 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (3)
docs/examples/portal.tsx (2)
8-33: 建议重构内置位置配置以提高可维护性建议将
builtinPlacements配置重构为更易维护的形式。可以考虑使用以下方式重构：
-const builtinPlacements = { - left: { - points: ['cr', 'cl'], - }, - right: { - points: ['cl', 'cr'], - }, - // ... more placements -}; +const POINTS = { + LEFT: ['cr', 'cl'], + RIGHT: ['cl', 'cr'], + TOP: ['bc', 'tc'], + BOTTOM: ['tc', 'bc'], +} as const; + +const builtinPlacements = { + left: { points: POINTS.LEFT }, + right: { points: POINTS.RIGHT }, + top: { points: POINTS.TOP }, + bottom: { points: POINTS.BOTTOM }, + topLeft: { points: ['bl', 'tl'] }, + topRight: { points: ['br', 'tr'] }, + bottomRight: { points: ['tr', 'br'] }, + bottomLeft: { points: ['tl', 'bl'] }, +};
69-107: 建议优化组件结构和样式处理当前实现存在以下问题：
- 内联样式可能影响性能
- 组件结构可以更加模块化
建议：
- 将样式抽离到单独的样式文件
- 将按钮组件抽离为独立的子组件
- 使用 CSS-in-JS 或 CSS Modules 替代内联样式
示例重构：
// styles.ts export const containerStyle = { padding: 100, display: 'flex', flexDirection: 'column' as const, alignItems: 'flex-start', gap: 100, }; // Button.tsx const ActionButton = ({ onClick, children }) => ( <button onClick={onClick} onMouseDown={(e) => { e.stopPropagation(); e.preventDefault(); }} > {children} </button> );src/Popup/index.tsx (1)
23-23: 建议添加属性文档注释为了提高代码可维护性，建议为新增的
onMouseDownCapture属性添加 JSDoc 注释，说明其用途和使用场景。建议添加如下注释：
+ /** + * 捕获阶段的鼠标按下事件处理器 + * 用于处理 Portal 关闭时的特殊场景 + */ onMouseDownCapture?: React.MouseEventHandler<HTMLDivElement>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
docs/demos/portal.md(1 hunks)docs/examples/portal.tsx(1 hunks)src/Popup/index.tsx(3 hunks)src/index.tsx(2 hunks)tests/basic.test.jsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/demos/portal.md
🔇 Additional comments (4)
src/Popup/index.tsx (1)
23-23: 实现看起来不错，建议验证 Portal 关闭场景！
新增的 onMouseDownCapture 事件处理实现清晰且符合 React 事件处理规范。这个改动应该是为了解决 Portal 关闭时的事件传播问题。
建议运行以下脚本来验证相关的 Portal 关闭用例：
Also applies to: 109-109, 260-260
✅ Verification successful
Let's gather more information about the test case and implementation details.
Based on the test cases and implementation details, I can now provide a final response:
onMouseDownCapture 实现正确且必要！
通过代码分析发现：
- 该事件处理是为了解决 Portal 场景下的点击关闭问题
- 在
src/index.tsx中的实现显示这是一个必要的补充，因为createPortal创建的元素不在 popup 元素内 - 使用捕获阶段事件可以确保在事件冒泡之前正确处理点击行为
测试用例 tests/basic.test.jsx 中的 Portal 相关测试验证了这个行为是正确的，特别是在处理嵌套 Portal 和点击关闭场景时。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：查找与 Portal 关闭相关的测试用例和实现
# 测试：搜索相关的测试文件
echo "查找相关测试用例："
rg -l "createPortal.*close" --glob "**/*.test.{ts,tsx,js,jsx}"
# 测试：搜索 Portal 相关的实现
echo "查找 Portal 相关实现："
rg "createPortal" --glob "**/*.{ts,tsx}"
Length of output: 681
Script:
#!/bin/bash
# 查看测试用例内容
echo "Portal 关闭测试用例内容："
cat tests/basic.test.jsx
# 查看 Popup 组件中的 Portal 相关实现
echo -e "\nPopup Portal 实现："
cat src/index.tsx
# 检查是否有其他使用 onMouseDownCapture 的地方
echo -e "\n其他 onMouseDownCapture 使用："
rg "onMouseDownCapture" -A 3 --glob "**/*.{ts,tsx}"
Length of output: 57581
src/index.tsx (2)
384-386: 代码格式优化良好！
状态声明的格式调整提高了代码的可读性。
725-732: 事件处理逻辑合理，修复了Portal关闭问题
这个改动通过在捕获阶段处理鼠标按下事件，巧妙地解决了createPortal场景下的关闭时序问题。使用捕获阶段确保了事件能在Portal内部处理之前被正确捕获。
运行以下脚本验证相关的事件处理逻辑：
✅ Verification successful
Let me analyze the search results. I can see that:
- The
onMouseDownCapturehandler in the review is part of the Popup component - There are related mouse event handlers in both test files and example files
- The main implementation in
src/index.tsxshows proper event handling coordination
I can now generate the final response in Chinese:
事件处理逻辑验证通过，确认实现合理
代码搜索结果显示：
- 在 Portal 场景下使用捕获阶段处理点击事件是合适的
- 相关的测试用例和示例代码都采用了类似的事件处理模式
- 主要实现中的事件处理流程完整且协调
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：检查是否存在其他鼠标事件处理相关的代码
# 测试：搜索相关的事件处理代码
rg -A 3 'onMouseDown|onClick|onMouseUp'
Length of output: 5782
tests/basic.test.jsx (1)
1202-1231: 测试用例实现完整且合理
测试用例很好地覆盖了 Portal 场景下的关键交互行为：
- 验证了点击 Portal 内容不会关闭弹出层
- 验证了点击外部区域会正确关闭弹出层
- 使用了异步等待来处理计时器
| React.useEffect(() => { | ||
| const button = buttonRef.current; | ||
| if (button) { | ||
| button.addEventListener('mousedown', (e) => { | ||
| console.log('button natives down'); | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| }); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
需要清理事件监听器以防止内存泄漏
useEffect 中添加的事件监听器在组件卸载时没有被移除。
建议修改如下：
React.useEffect(() => {
const button = buttonRef.current;
if (button) {
- button.addEventListener('mousedown', (e) => {
+ const handleMouseDown = (e: MouseEvent) => {
console.log('button natives down');
e.stopPropagation();
e.preventDefault();
- });
+ };
+
+ button.addEventListener('mousedown', handleMouseDown);
+
+ return () => {
+ button.removeEventListener('mousedown', handleMouseDown);
+ };
}
}, []);📝 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.
| React.useEffect(() => { | |
| const button = buttonRef.current; | |
| if (button) { | |
| button.addEventListener('mousedown', (e) => { | |
| console.log('button natives down'); | |
| e.stopPropagation(); | |
| e.preventDefault(); | |
| }); | |
| } | |
| }, []); | |
| React.useEffect(() => { | |
| const button = buttonRef.current; | |
| if (button) { | |
| const handleMouseDown = (e: MouseEvent) => { | |
| console.log('button natives down'); | |
| e.stopPropagation(); | |
| e.preventDefault(); | |
| }; | |
| button.addEventListener('mousedown', handleMouseDown); | |
| return () => { | |
| button.removeEventListener('mousedown', handleMouseDown); | |
| }; | |
| } | |
| }, []); |
| const PortalPopup = () => | ||
| createPortal( | ||
| <div | ||
| style={popupBorderStyle} | ||
| onMouseDown={(e) => { | ||
| console.log('Portal Down', e); | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| }} | ||
| > | ||
| i am a portal element | ||
| </div>, | ||
| document.body, | ||
| ); |
There was a problem hiding this comment.
建议改进门户组件的可访问性和事件处理
当前实现存在以下问题：
- 缺少必要的 ARIA 属性
- 事件处理可能影响键盘访问性
建议进行如下修改：
const PortalPopup = () =>
createPortal(
<div
style={popupBorderStyle}
+ role="dialog"
+ aria-modal="true"
+ tabIndex={-1}
onMouseDown={(e) => {
console.log('Portal Down', e);
- e.stopPropagation();
- e.preventDefault();
+ // 只在点击背景时阻止事件
+ if (e.target === e.currentTarget) {
+ e.stopPropagation();
+ e.preventDefault();
+ }
}}
+ onKeyDown={(e) => {
+ if (e.key === 'Escape') {
+ // 处理 ESC 关闭
+ }
+ }}
>
i am a portal element
</div>,
document.body,
);Committable suggestion skipped: line range outside the PR's diff.
| <Trigger | ||
| popupPlacement="right" | ||
| action={['click']} | ||
| builtinPlacements={builtinPlacements} | ||
| popup={ | ||
| <div style={popupBorderStyle}> | ||
| i am a click popup | ||
| <PortalPopup /> | ||
| </div> | ||
| } | ||
| onPopupVisibleChange={(visible) => { | ||
| console.log('visible change:', visible); | ||
| }} | ||
| > | ||
| <button>Click Me</button> | ||
| </Trigger> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议统一事件处理方式
目前组件中混合使用了多种事件处理方式（Trigger props、React 事件、原生事件），建议统一处理方式以提高可维护性。
建议将所有事件处理统一到 React 的方式：
<Trigger
popupPlacement="right"
action={['click']}
builtinPlacements={builtinPlacements}
+ destroyPopupOnHide
popup={
<div style={popupBorderStyle}>
i am a click popup
<PortalPopup />
</div>
}
onPopupVisibleChange={(visible) => {
console.log('visible change:', visible);
+ // 在这里统一处理弹窗状态
}}
>
- <button>Click Me</button>
+ <button onClick={(e) => {
+ // 统一的点击处理
+ console.log('trigger clicked');
+ }}>
+ Click Me
+ </button>
</Trigger>📝 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.
| <Trigger | |
| popupPlacement="right" | |
| action={['click']} | |
| builtinPlacements={builtinPlacements} | |
| popup={ | |
| <div style={popupBorderStyle}> | |
| i am a click popup | |
| <PortalPopup /> | |
| </div> | |
| } | |
| onPopupVisibleChange={(visible) => { | |
| console.log('visible change:', visible); | |
| }} | |
| > | |
| <button>Click Me</button> | |
| </Trigger> | |
| <Trigger | |
| popupPlacement="right" | |
| action={['click']} | |
| builtinPlacements={builtinPlacements} | |
| destroyPopupOnHide | |
| popup={ | |
| <div style={popupBorderStyle}> | |
| i am a click popup | |
| <PortalPopup /> | |
| </div> | |
| } | |
| onPopupVisibleChange={(visible) => { | |
| console.log('visible change:', visible); | |
| // 在这里统一处理弹窗状态 | |
| }} | |
| > | |
| <button onClick={(e) => { | |
| // 统一的点击处理 | |
| console.log('trigger clicked'); | |
| }}> | |
| Click Me | |
| </button> | |
| </Trigger> |
ref #434
Need test on antd since it may affected by animation.
Summary by CodeRabbit
Test，实现弹出界面和事件处理。Popup组件支持新的鼠标事件处理属性onMouseDownCapture。TriggerProps接口，简化API使用，并提供弃用通知。Trigger组件的测试用例，验证弹出行为。