feat: terminal adds drag-and-drop #4300
Conversation
|
|
|
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. 概述演练此拉取请求为终端应用程序引入了拖放功能。主要变更包括在 变更
序列图sequenceDiagram
participant User
participant TabItem
participant TerminalView
User->>TabItem: 开始拖动
TabItem->>TerminalView: 触发 onDragStart
User->>TabItem: 放置到新位置
TabItem->>TerminalView: 触发 onDrop
TerminalView->>TerminalView: 交换组
可能相关的 PR
建议审阅者
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/terminal-next/src/common/render.ts (1)
21-22: 建议在文档中补充 DragEvent 参数说明
这些新增的 drag-and-drop 回调属性很好地提升了可扩展性，但可在注释或文档中说明事件对象的使用方式，以便后续维护。packages/terminal-next/src/common/controller.ts (1)
143-144: 新增方法 swapGroup(i, j) 易用性良好，但需确保索引合法
交换功能对增强可交互性很有帮助。建议在下一步实现中对传入的索引 i、j 做边界检查，若索引不合法可进行相应的异常处理或日志记录。packages/terminal-next/src/browser/component/tab.item.tsx (1)
77-97: 拖拽操作的视觉提示“on-drag-over”需注意类名重复
当前通过添加和移除'on-drag-over'类名来进行视觉提示，如有其他组件也使用该类名，可能造成冲突。建议在命名上使用更具唯一性的前缀。-ref.current.classList.add('on-drag-over'); +ref.current.classList.add('terminal-on-drag-over');packages/terminal-next/src/browser/terminal.view.ts (1)
278-293: 建议完善越界检查以提高稳健性当前方法仅在
i或j等于 -1 时返回，但未对索引是否超出数组合法范围进行验证，可能导致在越界场景下出现不可预期的行为。可在交换前先检查i和j是否在newGroups数组有效范围内，确保方法逻辑更加健壮。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/terminal-next/src/browser/component/tab.item.tsx(1 hunks)packages/terminal-next/src/browser/component/tab.view.tsx(1 hunks)packages/terminal-next/src/browser/terminal.view.ts(1 hunks)packages/terminal-next/src/common/controller.ts(1 hunks)packages/terminal-next/src/common/render.ts(1 hunks)
🔇 Additional comments (2)
packages/terminal-next/src/common/render.ts (1)
25-25: 与其他属性保持一致的可读性
draggable属性的引入遵循了可选属性的风格，命名也恰当，建议在后续维护中确保与组件其他可选属性一致，便于统一管理。packages/terminal-next/src/browser/component/tab.view.tsx (1)
49-61: 注意在 onDrop 中处理默认行为与冒泡
在对拖拽的事件链进行处理时，最好检查是否需要调用stopPropagation()或preventDefault()以避免与上层或同级元素的事件冲突。当前实现已调用e.preventDefault()在 onDragOver（见下方），但 onDrop 同样可能需要此处理以确保事件不会被冒泡到其它元素。onDrop={(e: React.DragEvent) => { + e.preventDefault(); if (e.dataTransfer.getData('groupIndex')) { ... } }}✅ Verification successful
Let me search for related drag and drop event handling in the codebase to verify if there are any potential event conflicts.
Let me try a different search approach without specifying file types.
根据代码库搜索结果，我可以得出结论了。
建议在拖拽事件处理中添加 preventDefault() 以确保正确行为
从代码库中可以看到:
- 在
tab.item.tsx中的onDragOver已经调用了e.preventDefault()，这是正确的。- 在其他组件的拖拽实现中，如
editor.view.tsx、tab.view.tsx等，都同时使用了preventDefault()来阻止默认行为。- 特别是在
tab.view.tsx中，拖拽事件的处理不仅使用了preventDefault()，还使用了stopPropagation()来阻止事件冒泡。因此，为了保持一致性并确保拖拽功能的正确性，建议在
onDrop处理程序中添加preventDefault()：onDrop={(e: React.DragEvent) => { + e.preventDefault(); if (e.dataTransfer.getData('groupIndex')) { { const i = e.dataTransfer.getData('groupIndex'); view.swapGroup(Number(i), index); view.selectGroup(index); } } }}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for drag and drop event handlers in the codebase rg -A 5 "onDrop|onDragOver|onDragStart" --type tsx --type jsxLength of output: 91
Script:
#!/bin/bash # Search for drag and drop event handlers in the codebase rg -A 5 "onDrop|onDragOver|onDragStart"Length of output: 37022
|
ternimal -> terminal |
|
@lulusir 感谢 PR，麻烦签署一下 CLA |
lulusir
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4300 +/- ##
==========================================
- Coverage 54.21% 54.20% -0.01%
==========================================
Files 1634 1634
Lines 99917 99927 +10
Branches 21699 21700 +1
==========================================
+ Hits 54166 54167 +1
- Misses 38008 38016 +8
- Partials 7743 7744 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/terminal-next/src/browser/component/tab.item.tsx (2)
75-106: 优化拖拽处理器的实现建议进行以下优化：
- 将 'on-drag-over' CSS 类名定义在 CSS 模块中，保持一致性
- 添加错误处理，防止 DOM 操作失败
- 优化重复的 classList 操作
- 考虑使用 React 的 state 来管理拖拽状态，而不是直接操作 DOM
建议按如下方式重构:
+ const [isDragOver, setIsDragOver] = useState(false); - const handleDragOver = useCallback( - (e: React.DragEvent<HTMLDivElement>) => { - e.preventDefault(); - if (ref.current) { - ref.current.classList.add('on-drag-over'); - } - }, - [ref], - ); + const handleDragOver = useCallback((e: React.DragEvent<HTMLDivElement>) => { + e.preventDefault(); + setIsDragOver(true); + }, []); - const handleDragLeave = useCallback(() => { - if (ref.current) { - ref.current.classList.remove('on-drag-over'); - } - }, [ref]); + const handleDragLeave = useCallback(() => { + setIsDragOver(false); + }, []); - const handleDrop = useCallback( - (e: React.DragEvent<HTMLDivElement>) => { - if (ref.current) { - ref.current.classList.remove('on-drag-over'); - } - props.onDrop?.(e); - }, - [ref, props.onDrop], - ); + const handleDrop = useCallback((e: React.DragEvent<HTMLDivElement>) => { + setIsDragOver(false); + props.onDrop?.(e); + }, [props.onDrop]);然后在样式中添加:
+ .dragOver { + /* 定义拖拽悬停样式 */ + }在组件的 className 中使用:
className={cls({ [styles_item_container]: true, [styles_tab_item_selected]: !!props.selected, + [styles.dragOver]: isDragOver, })}
110-114: 优化拖拽事件绑定建议将拖拽事件处理器通过对象传递，提高代码可读性。
<div draggable={props.draggable} - onDragStart={handleDragStart} - onDragOver={handleDragOver} - onDragLeave={handleDragLeave} - onDrop={handleDrop} + {...{ + onDragStart: handleDragStart, + onDragOver: handleDragOver, + onDragLeave: handleDragLeave, + onDrop: handleDrop, + }} // ...其他属性 >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/terminal-next/src/browser/component/tab.item.tsx(1 hunks)packages/terminal-next/src/browser/component/tab.view.tsx(1 hunks)
Types
Background or solution
#4218
Summary by CodeRabbit
新功能
增强
技术改进