-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add onScroll event handling for Mentions dropdown #282
feat: add onScroll event handling for Mentions dropdown #282
Conversation
Walkthrough该拉取请求引入了多个更改,主要集中在 Changes
Possibly related PRs
📜 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
docs/examples/debug.tsx (2)
9-11
: 建议改进调试示例的实现方式当前的实现仅仅打印了事件对象,建议:
- 添加事件类型声明以提供更好的类型安全性
- 展示更实用的滚动事件处理示例
建议按照以下方式修改代码:
- onScroll={e => { - console.log(e); - }} + onScroll={(e: React.UIEvent<HTMLDivElement>) => { + // 展示更多有用的事件属性 + console.log('Scroll position:', { + scrollTop: e.currentTarget.scrollTop, + scrollHeight: e.currentTarget.scrollHeight, + clientHeight: e.currentTarget.clientHeight + }); + }}
Line range hint
1-30
: 建议添加更多示例说明作为调试示例文件,建议添加注释说明该组件的用途和滚动事件的使用场景。
建议在文件开头添加以下注释:
+// 此示例展示了 Mentions 组件的基本用法 +// 以及如何处理下拉列表的滚动事件 +// 可用于调试滚动行为或实现虚拟滚动等高级功能docs/examples/onScroll.tsx (2)
10-10
: 不建议在生产环境中直接使用 console.log直接使用
console.log
作为事件处理函数不适合生产环境。建议实现proper的事件处理函数。建议修改为:
- onDropdownScroll={console.log} + onDropdownScroll={(e) => { + // 在这里实现实际的滚动事件处理逻辑 + }}
6-18
: 建议添加类型定义作为TypeScript文件,建议为组件添加适当的类型定义,以提高代码的可维护性。
建议添加以下类型定义:
-export default () => ( +const OnScrollExample: React.FC = () => ( // ... existing implementation -); +); + +export default OnScrollExample;docs/demo.md (1)
43-46
: 文档结构清晰,建议补充使用说明新增的 On Scroll 部分保持了与其他章节一致的格式,这很好。不过建议在示例代码引用之前添加简短说明,解释该功能的使用场景和目的,这样能帮助用户更好地理解此特性。
建议添加如下说明文字:
## On Scroll +当下拉列表内容较多时,可以通过 `onDropdownScroll` 属性来监听滚动事件,实现动态加载等功能。 + <code src="./examples/onScroll.tsx"></code>tests/Mentions.spec.tsx (1)
303-312
: 测试用例实现正确但可以更全面基础功能测试已经实现,但建议增加以下测试场景以提高测试覆盖率:
- 验证回调函数的参数
- 测试不同滚动位置的情况
- 添加边缘情况测试(如空下拉列表)
建议参考以下示例扩展测试用例:
it('onDropdownScroll should work', () => { const onDropdownScroll = jest.fn(); const { container, baseElement } = renderMentions({ onDropdownScroll }); simulateInput(container, '@'); act(() => { jest.runAllTimers(); }); - fireEvent.scroll(baseElement.querySelector('.rc-mentions-dropdown-menu')); - expect(onDropdownScroll).toHaveBeenCalled(); + const dropdownMenu = baseElement.querySelector('.rc-mentions-dropdown-menu'); + + // 测试滚动事件参数 + fireEvent.scroll(dropdownMenu, { + target: { scrollTop: 100 } + }); + expect(onDropdownScroll).toHaveBeenCalledWith(expect.any(Event)); + + // 测试空列表场景 + const emptyMentions = renderMentions({ + onDropdownScroll, + options: [] + }); + simulateInput(emptyMentions.container, '@'); + act(() => { + jest.runAllTimers(); + }); + const emptyDropdown = emptyMentions.baseElement.querySelector('.rc-mentions-dropdown-menu'); + fireEvent.scroll(emptyDropdown); + expect(onDropdownScroll).toHaveBeenCalledTimes(2); });src/Mentions.tsx (1)
470-477
: 建议优化滚动事件处理器的性能当前实现在快速滚动时可能会触发过于频繁的回调,建议进行以下优化:
- 添加节流(throttle)控制以限制回调频率
- 缓存容器高度值以减少
getBoundingClientRect()
的调用次数建议参考以下实现:
+ import { throttle } from 'rc-util/lib/utils/throttleByRaf'; - const onInternalScroll: React.UIEventHandler<HTMLDivElement> = event => { + const onInternalScroll = React.useCallback( + throttle((event: React.UIEvent<HTMLDivElement>) => { const { currentTarget } = event; - const containerHeight = currentTarget.getBoundingClientRect().height; + // 使用 offsetHeight 性能更好 + const containerHeight = currentTarget.offsetHeight; const currentOffset = currentTarget.scrollTop; onDropdownScroll?.(event, containerHeight, currentOffset); - }; + }), + [onDropdownScroll], + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
docs/demo.md
(1 hunks)docs/examples/debug.tsx
(1 hunks)docs/examples/onScroll.less
(1 hunks)docs/examples/onScroll.tsx
(1 hunks)src/DropdownMenu.tsx
(2 hunks)src/Mentions.tsx
(4 hunks)src/MentionsContext.ts
(1 hunks)tests/Mentions.spec.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/examples/onScroll.less
🔇 Additional comments (8)
docs/examples/onScroll.tsx (2)
1-4
: 导入语句看起来没有问题!
导入语句的组织结构合理,遵循了先导入第三方库,然后是全局样式,最后是本地样式的最佳实践。
12-12
: 谨慎使用 open
属性
在示例代码中使用 open
属性强制下拉框保持打开状态可以理解,但在实际应用中可能会影响用户体验。
建议确认这是否仅用于演示目的。如果是实际生产代码,建议移除此属性让下拉框的开关状态由用户交互来控制。
src/MentionsContext.ts (1)
12-12
: 新增的 onScroll 事件处理器类型定义正确且符合最佳实践
类型定义遵循了与现有的 onFocus 和 onBlur 事件处理器相同的模式,使用 React.UIEventHandler 作为滚动事件的类型是恰当的。
让我们验证一下相关组件中的实现:
✅ Verification successful
onScroll 事件处理器已正确实现并集成到组件系统中
从代码验证结果可以看到:
MentionsContext
中定义的onScroll
事件处理器在DropdownMenu
组件中被正确使用Mentions
组件通过onInternalScroll
处理滚动事件并传递给上下文- 在示例代码中也有正确的使用演示
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述:验证 onScroll 在 DropdownMenu 和 Mentions 组件中的使用情况
# 检查 DropdownMenu 组件中的使用
ast-grep --pattern 'const { onScroll } = useContext(MentionsContext)'
# 检查 Mentions 组件中的实现
ast-grep --pattern 'onScroll={$_}'
Length of output: 108
Script:
#!/bin/bash
# 搜索包含 onScroll 的相关组件文件
rg -l "onScroll" --type ts --type tsx
# 检查具体实现细节
rg "onScroll" -A 3 -B 3 --type ts --type tsx
# 使用不同的模式搜索 MentionsContext 的使用
rg "MentionsContext" -A 3 -B 3 --type ts --type tsx
# 查找可能的滚动事件处理相关代码
ast-grep --pattern 'onScroll = ($_) => {
$$$
}'
Length of output: 270
Script:
#!/bin/bash
# 搜索包含 onScroll 的相关组件文件
rg -l "onScroll" --type typescript
# 检查具体实现细节
rg "onScroll" -A 3 -B 3 --type typescript
# 使用不同的模式搜索 MentionsContext 的使用
rg "MentionsContext" -A 3 -B 3 --type typescript
# 查找所有 .ts 和 .tsx 文件
fd "\.(ts|tsx)$"
Length of output: 833
Script:
#!/bin/bash
# 检查具体文件内容
cat src/DropdownMenu.tsx src/Mentions.tsx docs/examples/onScroll.tsx
# 使用 grep 直接搜索文件内容
grep -r "onScroll" src/ docs/examples/
Length of output: 20317
src/DropdownMenu.tsx (2)
22-22
: 从上下文中解构 onScroll 处理程序的实现看起来不错!
代码结构清晰,与现有的事件处理程序保持一致的模式。
38-38
: 建议添加 onScroll 事件的文档说明
虽然实现正确,但建议:
- 在组件文档中说明 onScroll 事件的具体用途和参数类型
- 确认滚动事件是否提供足够的信息给父组件使用
运行以下脚本来验证相关文档:
src/Mentions.tsx (3)
75-85
: 新增的 onDropdownScroll 属性定义完善!
属性定义包含了完整的 JSDoc 文档说明,参数类型定义准确,符合 TypeScript 最佳实践。
144-144
: 属性解构位置恰当!
onDropdownScroll
的解构位置符合代码现有的分组方式,与其他事件处理器属性放在一起。
510-510
: 上下文更新正确!
onScroll
处理器已正确添加到 MentionsContext.Provider 的 value 中,与其他事件处理器保持一致的模式。
options={Array.from({ length: 1000 }).map((_, index) => ({ | ||
value: `item-${index}`, | ||
label: `item-${index}`, | ||
}))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
建议优化下拉列表的性能
创建1000个选项可能会导致性能问题。建议:
- 考虑使用虚拟滚动来优化长列表的渲染
- 实现分页加载而不是一次性加载所有选项
建议重构为以下实现:
- options={Array.from({ length: 1000 }).map((_, index) => ({
- value: `item-${index}`,
- label: `item-${index}`,
- }))}
+ options={Array.from({ length: 50 }).map((_, index) => ({
+ value: `item-${index}`,
+ label: `item-${index}`,
+ }))}
+ onDropdownScroll={(e) => {
+ // 实现滚动加载更多选项的逻辑
+ }}
Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #282 +/- ##
==========================================
+ Coverage 98.38% 98.39% +0.01%
==========================================
Files 7 7
Lines 247 249 +2
Branches 59 59
==========================================
+ Hits 243 245 +2
Misses 4 4 ☔ View full report in Codecov by Sentry. |
ant-design/ant-design#51673
Summary by CodeRabbit
Mentions
组件添加了onScroll
事件处理程序,增强了用户交互。onScroll
组件,支持下拉菜单的滚动功能。DropdownMenu
组件现在支持滚动事件处理。Mentions
组件添加了针对onPopupScroll
事件处理程序的新测试用例。