feat: scrollTo add align support#1469
feat: scrollTo add align support#1469EmilyyyLiu wants to merge 7 commits intoreact-component:masterfrom
Conversation
|
Someone is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough为 Table 组件的公开方法 Changes
Sequence Diagram(s)sequenceDiagram
participant Button
participant TableRef
participant Container as ScrollContainer
participant Element as TargetElement
participant VirtualList
Button->>TableRef: call scrollTo({ index/key/top, offset?, align? })
TableRef->>Container: resolve scroll container & target element
alt target is in DOM (non-virtual)
alt no offset
TableRef->>Element: Element.scrollIntoView({ block: align ?? 'nearest' })
else offset provided
TableRef->>Container: compute targetTop using element.offsetTop/offsetHeight, clientHeight, scrollTop and align
TableRef->>Container: Container.scrollTo({ top: targetTop [+ offset] })
end
else virtual list
TableRef->>VirtualList: map align -> virtualAlign (top/bottom/auto) and pass { index/key, offset?, align: virtualAlign }
VirtualList->>VirtualList: perform virtual scrollTo with provided config
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 分钟 Possibly related PRs
Suggested reviewers
诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the align parameter to the scrollTo method of the Table component, allowing users to specify vertical alignment (start, center, end, nearest) when scrolling to a specific row. The changes include updates to the ScrollConfig interface, implementation logic in src/Table.tsx to handle alignment both with and without offsets, new documentation in the README, and comprehensive unit tests. Review feedback suggests translating README descriptions to English for consistency, updating outdated comments in the interface definition, and refining the alignMap type for better type safety and behavioral consistency with the documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/interface.ts (1)
39-47:⚠️ Potential issue | 🟡 Minor
offset的类型注释已经和新行为冲突了。这里还写着“设置
offset后总是顶对齐”，但本次改动已经允许align参与定位。这个类型是公开 API，IDE 悬浮提示会直接误导调用方；建议把说明改成“offset会叠加到按align计算出的目标位置”，或者如果只打算保留顶对齐，就把运行时逻辑收回到同一约束。✏️ 建议同步注释
/** * Additional offset in pixels to apply to the scroll position. * Only effective when using `key` or `index` mode. * Ignored when using `top` mode. - * When offset is set, the target element will always be aligned to the top of the container. + * In `key` / `index` mode, `offset` is added to the position resolved by `align`. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interface.ts` around lines 39 - 47, The JSDoc for the offset property is out-of-date: update the comment for offset (the offset?: number; declaration) to state that offset is added to the target position computed from align (ScrollLogicalPosition) rather than forcing top alignment; mention that offset is effective when using key/index modes and will be applied on top of the align calculation (or alternatively revert runtime behavior to force top alignment if you prefer keeping the old doc), and ensure the phrase "always aligned to the top" is removed so IDE hover shows the correct public API behavior for offset and align.src/Table.tsx (1)
354-380:⚠️ Potential issue | 🟠 Major
align: 'nearest'在 offset 分支里实际上没有被支持。Line 354 先把
align默认成了'nearest'，Line 377 又把'nearest'直接映射成elementTop。这样scrollTo({ index, offset, align: 'nearest' })和旧的scrollTo({ index, offset })完全等价，都会退化成顶对齐，而不是“最小滚动”。如果这里是想兼容老行为，建议保留align的原始undefined状态：未显式传入时继续走旧逻辑；显式传入'nearest'时再按当前scrollTop/elementBottom计算真正的 nearest。💡 一种兼容旧行为的修正方式
- const { index, top, key, offset, align = 'nearest' } = config; + const { index, top, key, offset, align } = config; if (validNumberValue(top)) { // In top mode, offset is ignored scrollBodyRef.current?.scrollTo({ top }); } else { const mergedKey = key ?? getRowKey(mergedData[index]); const targetElement = scrollBodyRef.current.querySelector( `[data-row-key="${mergedKey}"]`, ); if (targetElement) { if (!offset) { - targetElement.scrollIntoView({ block: align }); + targetElement.scrollIntoView({ block: align ?? 'nearest' }); } else { const container = scrollBodyRef.current; + const currentTop = container.scrollTop; const elementTop = (targetElement as HTMLElement).offsetTop; const elementHeight = (targetElement as HTMLElement).offsetHeight; + const elementBottom = elementTop + elementHeight; const containerHeight = container.clientHeight; + const containerBottom = currentTop + containerHeight; + const mergedAlign = align ?? 'start'; - const alignMap: Record<string, number> = { - start: elementTop, - end: elementTop + elementHeight - containerHeight, - center: elementTop + (elementHeight - containerHeight) / 2, - nearest: elementTop, - }; - const targetTop = alignMap[align] ?? elementTop; + let targetTop = elementTop; + switch (mergedAlign) { + case 'end': + targetTop = elementBottom - containerHeight; + break; + case 'center': + targetTop = elementTop - (containerHeight - elementHeight) / 2; + break; + case 'nearest': + if (elementTop < currentTop) { + targetTop = elementTop; + } else if (elementBottom > containerBottom) { + targetTop = elementBottom - containerHeight; + } else { + targetTop = currentTop; + } + break; + default: + targetTop = elementTop; + } container.scrollTo({ top: targetTop + offset }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Table.tsx` around lines 354 - 380, The current logic defaults align to 'nearest' and then treats 'nearest' as elementTop, which breaks true nearest behavior and conflates explicit nearest with the old undefined case; change the destructuring so align defaults to undefined (const { index, top, key, offset, align } = config), preserve legacy behavior when align is undefined (keep existing top-based calculation used by scrollTo({index, offset}) ), and implement real "nearest" handling inside the offset branch: compute container.scrollTop, elementTop and elementBottom, then choose the smallest scroll delta (top, center, or end) to produce the nearest targetTop when align === 'nearest'; update the alignMap/selection in the code that uses scrollBodyRef, mergedData, getRowKey and targetElement accordingly.
🧹 Nitpick comments (1)
tests/refs.spec.tsx (1)
114-186: 这组测试还区分不出align是否真的生效。现在的 mock 只记录了
scrollIntoView的目标元素，没有记录{ block }参数；而且 JSDOM 下offsetTop、offsetHeight、clientHeight默认都是0，所以start、center、end、nearest在这里基本会退化成同一种结果。实现就算把align忽略掉，这些用例大概率也还能通过。🧪 可参考的补强方向
let scrollParam: any = null; let scrollIntoViewElement: HTMLElement = null; +let scrollIntoViewParam: ScrollIntoViewOptions | undefined; beforeAll(() => { spyElementPrototypes(HTMLElement, { scrollTo: (_: any, param: any) => { scrollParam = param; }, - scrollIntoView() { + scrollIntoView(param?: ScrollIntoViewOptions) { // eslint-disable-next-line `@typescript-eslint/no-this-alias` scrollIntoViewElement = this; + scrollIntoViewParam = param; }, }); }); beforeEach(() => { scrollParam = null; scrollIntoViewElement = null; + scrollIntoViewParam = undefined; }); // Align start - should use scrollIntoView scrollIntoViewElement = null; ref.current.scrollTo({ index: 0, align: 'start' }); expect(scrollIntoViewElement.textContent).toEqual('light'); + expect(scrollIntoViewParam).toMatchObject({ block: 'start' });
align + offset这组再给 row / container 手动补一组不同的几何值后，才能真正把start、center、end、nearest的计算分开。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/interface.ts`:
- Around line 39-47: The JSDoc for the offset property is out-of-date: update
the comment for offset (the offset?: number; declaration) to state that offset
is added to the target position computed from align (ScrollLogicalPosition)
rather than forcing top alignment; mention that offset is effective when using
key/index modes and will be applied on top of the align calculation (or
alternatively revert runtime behavior to force top alignment if you prefer
keeping the old doc), and ensure the phrase "always aligned to the top" is
removed so IDE hover shows the correct public API behavior for offset and align.
In `@src/Table.tsx`:
- Around line 354-380: The current logic defaults align to 'nearest' and then
treats 'nearest' as elementTop, which breaks true nearest behavior and conflates
explicit nearest with the old undefined case; change the destructuring so align
defaults to undefined (const { index, top, key, offset, align } = config),
preserve legacy behavior when align is undefined (keep existing top-based
calculation used by scrollTo({index, offset}) ), and implement real "nearest"
handling inside the offset branch: compute container.scrollTop, elementTop and
elementBottom, then choose the smallest scroll delta (top, center, or end) to
produce the nearest targetTop when align === 'nearest'; update the
alignMap/selection in the code that uses scrollBodyRef, mergedData, getRowKey
and targetElement accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae9a085b-6c5c-49f6-88fe-5afb49c27bb0
⛔ Files ignored due to path filters (4)
tests/__snapshots__/ExpandRow.spec.jsx.snapis excluded by!**/*.snaptests/__snapshots__/FixedColumn.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Summary.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Table.spec.jsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
README.mddocs/examples/scrollY.tsxsrc/Table.tsxsrc/interface.tstests/refs.spec.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/interface.ts`:
- Around line 43-47: The public ScrollConfig.align option is being ignored in
the virtual path: update the virtual scroller code (the BodyGrid virtual branch
that handles offset/align in the scrollTo logic) to respect the
ScrollConfig.align value instead of forcibly setting align = 'top'; ensure the
scrollTo call (and any helper that computes target offset) reads the external
align and offset fields from ScrollConfig and computes the final scroll position
accordingly so scrollTo({ index, align: 'center', offset: 10 }) behaves as
documented. Reference symbols: ScrollConfig, align, offset, scrollTo, BodyGrid
(virtual branch).
In `@src/Table.tsx`:
- Around line 365-380: The current offset-branch mapping in Table.tsx maps align
'nearest' to elementTop causing unnecessary jumps; change the 'nearest' case in
the alignMap (or replace alignMap with conditional logic) to compute the minimal
scroll needed: read container.scrollTop and containerHeight, elementTop and
elementBottom (= elementTop + elementHeight), and if elementTop < scrollTop set
targetTop = elementTop, else if elementBottom > scrollTop + containerHeight set
targetTop = elementBottom - containerHeight, else set targetTop = scrollTop (no
change); then use container.scrollTo({ top: targetTop + offset }) so 'nearest'
only scrolls when the row is outside the viewport.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7624530-43b1-4628-a4fc-e07079cf3fde
📒 Files selected for processing (3)
README.mdsrc/Table.tsxsrc/interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
- Add VirtualScrollConfig type with Exclude<ScrollLogicalPosition, 'center'> - Implement align mapping for virtual table (start->top, end->bottom, nearest->auto) - Add default align 'auto' when neither align nor offset provided - Add backward compatibility: default to 'top' when offset provided but align not - Update tests to cover align parameter scenarios - Fix offset JSDoc comment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Virtual.spec.tsx (1)
427-450: 建议补充align: 'center'（或非法值）契约测试当前只覆盖了
start/end/nearest。建议再补一条center（或非法 align）断言，明确是“降级为auto”还是“直接拒绝”，避免后续回归到隐式行为。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Virtual.spec.tsx` around lines 427 - 450, Add a test case calling tblRef.current.scrollTo with align: 'center' (and another with an invalid align like 'invalid') and assert how the implementation handles them by checking global.scrollToConfig; specifically call scrollTo({ index: 50, align: 'center' }) and expect global.scrollToConfig.align === 'auto' (and likewise for the invalid value) so the behavior is explicit; place these assertions alongside the existing scrollTo tests that reference scrollTo, Reference, getTable and global.scrollToConfig and use await waitFakeTimer() before each expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/VirtualTable/BodyGrid.tsx`:
- Around line 90-97: alignMap[align] can be undefined if align holds an
unexpected value, so update the virtualAlign computation in BodyGrid (the
variables alignMap, virtualAlign, align, offset) to always produce a valid 'top'
| 'bottom' | 'auto' by using a safe fallback: use the mapped value only if
present (e.g., alignMap[align] !== undefined) otherwise fall back to offset ?
'top' : 'auto' (or default 'auto'), ensuring scrollTo always receives a valid
string.
---
Nitpick comments:
In `@tests/Virtual.spec.tsx`:
- Around line 427-450: Add a test case calling tblRef.current.scrollTo with
align: 'center' (and another with an invalid align like 'invalid') and assert
how the implementation handles them by checking global.scrollToConfig;
specifically call scrollTo({ index: 50, align: 'center' }) and expect
global.scrollToConfig.align === 'auto' (and likewise for the invalid value) so
the behavior is explicit; place these assertions alongside the existing scrollTo
tests that reference scrollTo, Reference, getTable and global.scrollToConfig and
use await waitFakeTimer() before each expect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81dac262-9019-4940-8d21-1af4a9856b5f
📒 Files selected for processing (3)
src/VirtualTable/BodyGrid.tsxsrc/interface.tstests/Virtual.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/interface.ts
- Fix nearest align to compute position after offset, then determine if scrolling is needed - Add align test buttons to scrollY and virtual demo files - Update center calculation formula Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Table.tsx (1)
377-395:⚠️ Potential issue | 🟠 Major
offset分支的默认对齐和offset叠加语义还没有和公开 API 对齐。Line 394 在
align未传时回退到了start，所以scrollTo({ index, offset })会跳成顶对齐，而不是这次 API 声明的默认nearest。另外 Line 386 在目标行已经可见时直接返回currentTop，会把align: 'nearest'下的offset静默吞掉。这里更稳妥的做法是先算“不带offset的基准位置”，最后统一再+ offset。🔧 建议修改
- if (align === 'nearest') { - const targetWithOffset = elementTop + offset; - const targetBottomWithOffset = elementBottom + offset; - - if (targetWithOffset < currentTop) { - targetTop = targetWithOffset; - } else if (targetBottomWithOffset > viewportBottom) { - targetTop = targetBottomWithOffset - containerHeight; - } else { - targetTop = currentTop; - } - } else { - const alignMap: Record<string, number> = { + const mergedAlign = align ?? 'nearest'; + + if (mergedAlign === 'nearest') { + let baseTop = currentTop; + + if (elementTop < currentTop) { + baseTop = elementTop; + } else if (elementBottom > viewportBottom) { + baseTop = elementBottom - containerHeight; + } + + targetTop = baseTop + offset; + } else { + const alignMap: Record<'start' | 'end' | 'center', number> = { start: elementTop, end: elementBottom - containerHeight, center: elementTop - (containerHeight - elementHeight) / 2, }; - targetTop = alignMap[align ?? 'start'] + offset; + targetTop = alignMap[mergedAlign] + offset; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Table.tsx` around lines 377 - 395, The current logic computes targetTop mixed with offset and falls back to 'start' when align is undefined, and the 'nearest' branch returns currentTop without applying offset when the element is already visible; change behavior so the default align is 'nearest' when align is undefined and compute a baseline target (without offset) for all branches, then add offset once at the end; specifically update the 'nearest' branch to set a baseTargetTop (use elementTop, elementBottom, currentTop, viewportBottom, containerHeight to decide baseTargetTop) instead of returning currentTop directly, change the alignMap fallback from 'start' to 'nearest', and finally set targetTop = baseTargetTop + offset (using the existing symbols elementTop, elementBottom, elementHeight, containerHeight, currentTop, viewportBottom, align, offset, targetTop).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Table.tsx`:
- Around line 377-395: The current logic computes targetTop mixed with offset
and falls back to 'start' when align is undefined, and the 'nearest' branch
returns currentTop without applying offset when the element is already visible;
change behavior so the default align is 'nearest' when align is undefined and
compute a baseline target (without offset) for all branches, then add offset
once at the end; specifically update the 'nearest' branch to set a baseTargetTop
(use elementTop, elementBottom, currentTop, viewportBottom, containerHeight to
decide baseTargetTop) instead of returning currentTop directly, change the
alignMap fallback from 'start' to 'nearest', and finally set targetTop =
baseTargetTop + offset (using the existing symbols elementTop, elementBottom,
elementHeight, containerHeight, currentTop, viewportBottom, align, offset,
targetTop).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5f6d899-b12b-4a48-94d1-b9f624dc1584
📒 Files selected for processing (3)
docs/examples/scrollY.tsxdocs/examples/virtual.tsxsrc/Table.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/examples/scrollY.tsx
Use nullish coalescing for safer align fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@gemini-code-assist CR again |
There was a problem hiding this comment.
Code Review
This pull request introduces an align parameter to the scrollTo method in the Table component, supporting both standard and virtualized tables. The implementation allows users to specify vertical alignment (start, center, end, nearest) when scrolling to a specific row by index or key. The logic for calculating scroll positions with offsets has been updated to respect these alignment options. Documentation, examples, and comprehensive tests have been added to cover the new functionality. Feedback suggests optimizing performance by moving static alignment mappings outside of the scrollTo function calls.
| start: elementTop, | ||
| end: elementBottom - containerHeight, | ||
| center: elementTop - (containerHeight - elementHeight) / 2, | ||
| }; | ||
| targetTop = alignMap[align ?? 'start'] + offset; |
There was a problem hiding this comment.
The alignMap object is recreated on every call to scrollTo. For better performance, consider moving this mapping outside the function or using a constant.
| const alignMap: Record<string, 'top' | 'bottom' | 'auto'> = { | ||
| start: 'top', | ||
| end: 'bottom', | ||
| nearest: 'auto', | ||
| }; |
There was a problem hiding this comment.
The alignMap object is recreated on every call to scrollTo. For better performance, consider moving this mapping outside the function or using a constant.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1469 +/- ##
==========================================
- Coverage 96.10% 96.05% -0.06%
==========================================
Files 57 57
Lines 3442 3473 +31
Branches 637 642 +5
==========================================
+ Hits 3308 3336 +28
- Misses 129 132 +3
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
为 Table 组件的
scrollTo方法增加align参数支持，类似于虚拟列表中的对齐方式。Feature
align: 'start'- 滚动到顶部align: 'center'- 滚动到中间align: 'end'- 滚动到底部align: 'nearest'- 智能滚动（默认）Related Issue
ant-design/ant-design#45945
Usage
也可以结合 offset 使用：
Summary by CodeRabbit
发版说明