-
-
Notifications
You must be signed in to change notification settings - Fork 217
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: add 1px tolerance for autoScroll #336
fix: add 1px tolerance for autoScroll #336
Conversation
📝 WalkthroughWalkthrough此次变更在 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 (
|
Bundle ReportBundle size has no change ✅ |
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 (2)
components/bubble/BubbleList.tsx (2)
40-41
: 建议为 TOLERANCE 常量添加注释说明建议添加注释来解释这个容差值的用途和选择 1px 作为阈值的原因,这样可以帮助其他开发者更好地理解代码。
+// 滚动位置计算的容差值(像素),用于解决由于浮点数计算误差导致的自动滚动问题 const TOLERANCE = 1;
94-96
: 滚动计算逻辑的改进建议当前实现正确地解决了自动滚动的问题,使用
Math.abs
处理滚动值是个好做法。不过,为了提高代码的可读性,建议将计算逻辑提取为一个命名良好的变量。+ const currentScrollDistance = target.scrollHeight - Math.abs(target.scrollTop) - target.clientHeight; + setScrollReachEnd(currentScrollDistance <= TOLERANCE); - setScrollReachEnd( - target.scrollHeight - Math.abs(target.scrollTop) - target.clientHeight <= TOLERANCE, - );
setScrollReachEnd( | ||
target.scrollHeight - Math.abs(target.scrollTop) - target.clientHeight <= TOLERANCE, | ||
); |
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.
💡 Codebase verification
发现了不一致的滚动计算逻辑
在代码库中发现了两种不同的滚动位置计算实现:
components/bubble/BubbleList.tsx
使用scrollHeight - Math.abs(scrollTop) - clientHeight <= TOLERANCE
components/attachments/FileList/index.tsx
使用scrollHeight - clientHeight !== scrollTop
建议统一这两处实现的计算逻辑,以保持代码库的一致性:
- 在
FileList
组件中也建议使用Math.abs(scrollTop)
来处理滚动位置 - 考虑引入相同的
TOLERANCE
值来处理误差
🔗 Analysis chain
建议验证其他组件中的滚动计算逻辑
为了保持代码库的一致性,建议检查其他组件中是否存在类似的滚动位置计算逻辑,可能也需要应用相同的优化。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 搜索代码库中其他可能需要类似优化的滚动位置计算
rg -l "scrollHeight.*scrollTop.*clientHeight" --type tsx --type jsx
# 查找其他可能的滚动到底部检测实现
ast-grep --pattern 'scrollHeight - $_ <= $_'
Length of output: 144
Script:
#!/bin/bash
# Search for scroll-related calculations in all files
rg "scrollHeight.*scrollTop.*clientHeight"
# Search for scroll position detection patterns
rg "scrollHeight.*clientHeight"
# Search for scroll-end detection implementations
rg "setScrollReachEnd|isScrollEnd"
Length of output: 826
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
=======================================
Coverage 91.36% 91.36%
=======================================
Files 66 66
Lines 1447 1448 +1
Branches 368 383 +15
=======================================
+ Hits 1322 1323 +1
Misses 125 125 ☔ View full report in Codecov by Sentry. |
非常感谢积极参与!Attachment 组件也是踩了同样的坑~感谢你,我的朋友~ |
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.
感谢贡献!
修复:
BubbleList中autoScroll功能无法按预期运行的问题。
解决方案:
容忍1px以内的误差。
详见issue: #317
Summary by CodeRabbit