fix: Adjust the top height of the proxy to solve the sliding issue #1310
Conversation
|
@Wxh16144 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Wxh16144, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves a scrolling issue in virtualized tables where the antd scrollTo utility failed to correctly scroll vertically. The problem stemmed from the virtual table component forwarding a ref as an object rather than a direct DOM element, preventing standard scrollTop manipulation. The fix introduces a proxied scrollTop property on this forwarded ref object, enabling proper vertical scrolling by delegating to the underlying virtual list's scroll methods.
Highlights
- Virtual Table Scrolling Fix: Addresses a bug where antd's scrollTo function could not properly scroll virtualized tables vertically due to the ref being an object instead of a DOM element.
- scrollTop Proxy Implementation: Implemented a scrollTop getter and setter on the ref object returned by VirtualTable/BodyGrid.tsx. This proxies the scrollTop property to the internal rc-virtual-list component's scrollTo and getScrollInfo methods.
- New Test Case for scrollTop: Added a dedicated test in tests/Virtual.spec.tsx to ensure the new scrollTop proxy functions correctly, allowing both reading and setting of the vertical scroll position.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a scrolling issue in virtual tables by proxying the scrollTop property, similar to how scrollLeft is handled. The implementation in BodyGrid.tsx is clean and effective. The accompanying tests in Virtual.spec.tsx validate the new functionality. I've identified a potential issue with test state pollution in the test setup and have provided a suggestion to fix it. Overall, this is a good fix.
| global.scrollToConfig = vi.fn(identity); | ||
| // global.collectGetScrollInfoReturn.mockReset(); |
There was a problem hiding this comment.
There are a couple of issues in the test setup within beforeEach that could lead to problems:
- Test State Pollution: The new test
should get and set scrollTop correctlymodifies the globalcollectGetScrollInfoReturnfunction. This change persists across tests, which can cause other tests to fail or behave unpredictably depending on execution order. This should be reset to its defaultidentityvalue before each test. - Incorrect Mock Initialization:
global.scrollToConfigis being initialized as a mock function (vi.fn(identity)), but its purpose is to hold the configuration object passed to thescrollTomethod. It should be reset tonullas it was previously to accurately reflect its intended use.
The suggested change below addresses both of these points.
| global.scrollToConfig = vi.fn(identity); | |
| // global.collectGetScrollInfoReturn.mockReset(); | |
| global.scrollToConfig = null; | |
| global.collectGetScrollInfoReturn = identity; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## antd-5.x #1310 +/- ##
============================================
- Coverage 97.78% 97.74% -0.05%
============================================
Files 77 77
Lines 7595 7618 +23
Branches 1153 1157 +4
============================================
+ Hits 7427 7446 +19
- Misses 162 166 +4
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Wxh16144
force-pushed
the
wuxh/fix-antd-54734
branch
from
c4de7b7 to
7a10759
Compare
|
background: ant-design/ant-design#54734
Reason
antd 侧在 https://github.com/ant-design/ant-design/blob/ddd525c7891fe94123e6c5b160f80ed630e8de26/components/table/InternalTable.tsx#L279-L281 使用了元素进行滑动操作。 scrollTo 源码可以知道。 如果传递的容器不是一个 html 最终会通过设置 scrollTop 来实现滑动。
Solution
普通表格中， ref 转发出去的是一个真实的 dom 。所以可以通过 scrollTop 方法进行滑动。
而在虚拟表中， 转发出去的 ref 是一个对象
所以我们需要像
scrollLeft一样，代理一个scrollTop来解决无法滑动的问题