-
-
Notifications
You must be signed in to change notification settings - Fork 595
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
fix: virtual scroll ref #1217
fix: virtual scroll ref #1217
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
变更概览演练此次更改主要涉及 变更
对链接问题的评估
可能相关的 PR
建议的审阅者
诗歌
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 #1217 +/- ##
=======================================
Coverage 97.76% 97.76%
=======================================
Files 75 76 +1
Lines 7427 7443 +16
Branches 1126 1128 +2
=======================================
+ Hits 7261 7277 +16
Misses 160 160
Partials 6 6 ☔ View full report in Codecov by Sentry. |
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: 0
🧹 Nitpick comments (5)
src/utils/offsetUtil.ts (3)
3-3
: 注释需要更新注释 "Copy from
rc-util/Dom/css.js
" 应该包含更多上下文信息,比如为什么要复制这段代码而不是直接使用原始库。
9-9
: 建议移除过时的浏览器兼容性注释IE8 的兼容性注释已经过时,建议移除或更新这个注释以反映当前的浏览器支持情况。
4-20
: 建议增强类型安全性和错误处理函数实现基本正确,但建议做以下改进:
- 添加对
getDOM
返回值的空值检查- 为
box
和docElem
添加类型断言export function getOffset(node: HTMLElement | Window) { const element = getDOM(node); + if (!element) { + throw new Error('无法获取 DOM 元素'); + } const box = element.getBoundingClientRect(); const docElem = document.documentElement; + if (!docElem) { + throw new Error('无法获取 document.documentElement'); + } return { left: box.left + (window.pageXOffset || docElem.scrollLeft) - (docElem.clientLeft || document.body.clientLeft || 0), top: box.top + (window.pageYOffset || docElem.scrollTop) - (docElem.clientTop || document.body.clientTop || 0), }; }tests/Sticky.spec.jsx (2)
59-59
: 测试用例命名需要更有描述性将测试用例从 'Sticky scroll' 重命名为 'Sticky scroll1' 不够描述性。建议使用更有意义的名称来描述测试场景。
Line range hint
59-201
: 建议重构测试代码以提高可维护性测试代码中存在重复的设置逻辑,建议提取共用的测试数据和设置到辅助函数中:
- 提取模拟的滚动值和事件
- 创建通用的表格数据生成器
+const createMockScrollData = () => ({ + scrollLeft: 100, + scrollTop: 100, + scrollWidth: 200, + clientWidth: 100, + offsetHeight: 100, +}); + +const createTestTableData = (count) => + Array.from({ length: count }, (_, i) => ({ + light: 'bamboo', + bamboo: 'light', + key: i + 1, + }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json
(1 hunks)src/stickyScrollBar.tsx
(1 hunks)src/utils/offsetUtil.ts
(1 hunks)tests/Sticky.spec.jsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/stickyScrollBar.tsx
🔇 Additional comments (1)
package.json (1)
60-60
: 依赖更新看起来合适rc-util 的版本更新(^5.41.0 -> ^5.44.3)与当前的改动相符,支持新的 DOM 操作功能。
@@ -0,0 +1,20 @@ | |||
import { getDOM } from 'rc-util/lib/Dom/findDOMNode'; | |||
|
|||
// Copy from `rc-util/Dom/css.js` |
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.
为啥要 copy 过来?
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.
在 v6 里,rc-util
所有的 .js
后缀代码都会删掉。这个 offset 可以抽成其他方法在 rc-virtual-list 里复用,所以临时复制出来。重构的时候换个新的。
fix ant-design/ant-design#52276
Summary by CodeRabbit
依赖更新
rc-util
依赖版本从^5.41.0
升级到^5.44.3
新功能
getOffset
实用函数,用于跨浏览器兼容的元素偏移计算测试