feat: scrollTo add align support#1469
Open
EmilyyyLiu wants to merge 4 commits intoreact-component:masterfrom
Open
feat: scrollTo add align support#1469EmilyyyLiu wants to merge 4 commits intoreact-component:masterfrom
EmilyyyLiu wants to merge 4 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. |
Walkthrough为 Table 组件的公开方法 Changes
Sequence Diagram(s)sequenceDiagram
participant Button
participant TableRef
participant Container as ScrollContainer
participant Element as TargetElement
Button->>TableRef: invoke scrollTo({ key/index/top, offset?, align? })
TableRef->>Container: resolve target element (by key/index or DOM query)
alt offset not provided
TableRef->>Element: Element.scrollIntoView({ block: align })
else offset provided
TableRef->>Container: compute targetTop based on element.offsetTop, offsetHeight, clientHeight and align
TableRef->>Container: Container.scrollTo({ top: targetTop + offset })
end
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 分钟 Possibly related PRs
Suggested reviewers
功能概览向 Table 组件的 变更明细
预估代码审查工作量🎯 3 (中等) | ⏱️ ~20 分钟 可能相关的 PR
建议审查人员
诗
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
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
Comment on lines
+43
to
+47
| * When offset is set, the target element will be aligned according to the `align` parameter. | ||
| */ | ||
| offset?: number; | ||
|
|
||
| align?: ScrollLogicalPosition; |
There was a problem hiding this comment.
align 的公共契约还没有覆盖到 virtual 路径。
这里已经把 align 暴露成 ScrollConfig 的公共字段，但 src/VirtualTable/BodyGrid.tsx:82-91 的 offset 分支仍然会把它覆盖成 'top'。这样在 virtual 表格里，scrollTo({ index, align: 'center', offset: 10 }) 依然会按顶部对齐，和这次新增 API 的语义不一致。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/interface.ts` around lines 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).
Comment on lines
365
to
+380
| if (!offset) { | ||
| // No offset, use scrollIntoView for default behavior | ||
| targetElement.scrollIntoView(); | ||
| targetElement.scrollIntoView({ block: align }); | ||
| } else { | ||
| // With offset, use element's offsetTop + offset | ||
| const container = scrollBodyRef.current; | ||
| const elementTop = (targetElement as HTMLElement).offsetTop; | ||
| scrollBodyRef.current.scrollTo({ top: elementTop + offset }); | ||
| const elementHeight = (targetElement as HTMLElement).offsetHeight; | ||
| const containerHeight = container.clientHeight; | ||
|
|
||
| const alignMap: Record<ScrollLogicalPosition, number> = { | ||
| start: elementTop, | ||
| end: elementTop + elementHeight - containerHeight, | ||
| center: elementTop + (elementHeight - containerHeight) / 2, | ||
| nearest: elementTop, | ||
| }; | ||
| const targetTop = alignMap[align] ?? elementTop; | ||
| container.scrollTo({ top: targetTop + offset }); |
There was a problem hiding this comment.
nearest 在 offset 分支里退化成了强制顶对齐。
Line 377 把 nearest 映射成了 elementTop，所以 scrollTo({ index, offset }) 或显式传 align: 'nearest' 时，会无条件跳到目标行顶部附近，而不是“仅在需要时滚到最近可见位置”。这和这次新增的默认语义不一致，也会让本来已经可见的行发生意外跳动。
💡 可行修正
} else {
const container = scrollBodyRef.current;
const elementTop = (targetElement as HTMLElement).offsetTop;
const elementHeight = (targetElement as HTMLElement).offsetHeight;
const containerHeight = container.clientHeight;
+ const currentTop = container.scrollTop;
+ const elementBottom = elementTop + elementHeight;
+ const viewportBottom = currentTop + containerHeight;
- const alignMap: Record<ScrollLogicalPosition, number> = {
+ const alignMap: Record<Exclude<ScrollLogicalPosition, 'nearest'>, number> = {
start: elementTop,
end: elementTop + elementHeight - containerHeight,
center: elementTop + (elementHeight - containerHeight) / 2,
- nearest: elementTop,
};
- const targetTop = alignMap[align] ?? elementTop;
+ const targetTop =
+ align === 'nearest'
+ ? elementTop < currentTop
+ ? elementTop
+ : elementBottom > viewportBottom
+ ? elementBottom - containerHeight
+ : currentTop
+ : alignMap[align];
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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
发版说明
新功能
scrollTo支持可选align（start/center/end/nearest），用于更精确的滚动对齐行为。文档
scrollTo方法说明与用法示例，更新了 ScrollConfig 参数说明以包含align。示例
align值滚动表格（start/center/end）。测试
scrollTo的align行为与带 offset 的滚动计算添加了测试覆盖。