feat: support expand all control in expandable column header - #1505
feat: support expand all control in expandable column header#1505nikzanda wants to merge 12 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@nikzanda is attempting to deploy a commit to the afc163's projects 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:
Walkthrough新增展开全部行功能。该功能支持默认或自定义图标、批量展开与收起、可展开行过滤、受控状态同步和自定义展开列表头。README 文档同步更新。 Changes展开全部行支持
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Table
participant useExpand
participant useColumns
participant ExpandIcon
Table->>useExpand: 获取展开全部状态和处理器
useExpand-->>Table: 返回 expandAllInfo
Table->>useColumns: 传递 ExpandIcon 和 expandAllInfo
useColumns->>ExpandIcon: 渲染 type="all" 图标
ExpandIcon->>useExpand: 触发展开或收起全部行
useExpand-->>Table: 更新 expandedRowKeys
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
22bede1 to
ef33496
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1505 +/- ##
==========================================
+ Coverage 99.05% 99.09% +0.03%
==========================================
Files 45 45
Lines 1376 1429 +53
Branches 413 433 +20
==========================================
+ Hits 1363 1416 +53
Misses 13 13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/expandUtil.tsx`:
- Around line 57-64: Update the expand-all control in the rendered component
from a span to a native button with type="button", preserving its className and
onClick behavior. Add an aria-label that reflects the current expanded state and
bind aria-expanded to expanded. Extend the interaction tests to verify Tab focus
and activation via Enter and Space.
🪄 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 Plus
Run ID: 3313343f-3c8c-4313-babe-050aa6c7b4e7
📒 Files selected for processing (8)
README.mdREADME.zh-CN.mdsrc/Table.tsxsrc/hooks/useColumns/index.tsxsrc/hooks/useExpand.tssrc/interface.tssrc/utils/expandUtil.tsxtests/ExpandRow.spec.jsx
|
@afc163 @zombieJ, the implementation is now complete and this PR is ready for review. It includes the expand-all API, controlled and uncontrolled behavior, When you have time, could you please review the proposed API and implementation? I’m happy to make any adjustments needed before proceeding with the Ant Design integration for ant-design/ant-design#8791. Thanks! |
|
@zombieJ, gentle follow-up on this PR. I’ve addressed your latest feedback by changing columnTitle={({ expandIcon }) => (
<div>{expandIcon}</div>
)}The typings, documentation, and regression tests have also been updated. The test suite, type checks, lint, and coverage checks are passing. When you have time, could you please take another look and let me know whether the API now matches what you had in mind? I’m happy to make any further adjustments. Thanks! |
| expandedRowRender: ExpandedRowRender<RecordType>; | ||
| forceRender: boolean; | ||
| expandIcon: RenderExpandIcon<RecordType>; | ||
| ExpandIcon?: ExpandIconComponent<RecordType>; |
There was a problem hiding this comment.
Table Context give the getComponent ability. No need additional pass the ExpandIcon.
Or we just merge the expandIcon & ExpandIcon with mergedExpandIcon to handle both components def or legacy expandIcon def.
There was a problem hiding this comment.
Thanks for the suggestion, @zombieJ. I followed the second approach and merged the component and legacy definitions into the internal mergedExpandIcon.
The resolution order is now:
components.ExpandIconexpandable.expandIconas the deprecated row-only fallback- the default expand icon
TableContext now exposes only the resolved expandIcon; the additional ExpandIcon field and its row-level propagation have been removed. Both expanded-row tables and nested/virtual rows consume the same resolved renderer.
The raw components.ExpandIcon is still used directly only for the header control with type="all", since the legacy expandable.expandIcon API applies only to row icons.
| }, [expandableType, getRowKey, mergedData, rowExpandable, showExpandAll]); | ||
|
|
||
| const allExpanded = | ||
| expandableRows.length > 0 && expandableRows.every(({ key }) => mergedExpandedKeys.has(key)); |
There was a problem hiding this comment.
This will cause perf cost when large data.
And above rowExpandable maybe not stable object in parent render:
<VirtualTable
data={source.filter(...)}
expandable={{
showExpandAll: true,
rowExpandable: record => ...
}}
/>We can move the allExpanded into memo also and remove rowExpandable from the deps(in most case it's safe).
There was a problem hiding this comment.
Updated as suggested:
- memoized
allExpandedseparately; - intentionally removed
rowExpandablefrom theexpandableRowsdependencies; - added a regression test covering unstable inline callbacks and data changes.
| [getRowKey, mergedExpandedKeys, mergedData, onExpand, onExpandedRowsChange], | ||
| ); | ||
|
|
||
| const onTriggerExpandAll: React.MouseEventHandler<HTMLElement> = React.useCallback( |
There was a problem hiding this comment.
change to rc util useEvent which will not need deps
nrps9909
left a comment
There was a problem hiding this comment.
I reviewed the current head, 779d2877ac82309d4ea24554989982e66f757f35, and found one correctness blocker in the new expand-all state.
expandableRows intentionally omits rowExpandable from its dependency list. That avoids rescanning for an equivalent inline callback, but it also keeps the old eligible-row set when the callback's result legitimately changes while data keeps the same identity. Row rendering calls the latest predicate, so the row controls and the expand-all control then disagree: the header can report itself expanded and emit keys for a row that is now non-expandable, while the newly eligible row is ignored.
I reproduced this on the exact head by extending the existing test with the same stable sampleData:
const { container, rerender } = render(renderTable(({ key }) => key === 0));
rerender(renderTable(({ key }) => key === 1));
fireEvent.click(container.querySelector('.expand-all-icon'));
expect(onExpandAll).toHaveBeenLastCalledWith(true, [sampleData[1]]);
expect(onExpandedRowsChange).toHaveBeenLastCalledWith([1]);The test fails because both callbacks receive row/key 0, from the previous predicate. This is a realistic pattern when an inline rowExpandable closes over component state or permissions. The current regression test only swaps in a new callback with identical key === 1 behavior, so it cannot catch the stale-semantics case.
Please make the cached eligible-row set invalidate when rowExpandable semantics change (including the callback in the memo dependencies is the direct correctness fix), and change/add a test where the replacement predicate selects a different row. If preserving the large-data optimization requires another structure, it still needs to keep the header state, callback records, and row-level rowExpandable result synchronized.
Validation on this head: the focused ExpandRow suite passed 49/49 before the probe; the full suite passed 21 files / 236 tests; tsc, lint (existing warnings only), compile, and git diff --check passed.
AI assistance disclosure: Codex was used to trace the memoized eligibility path, inspect all current review threads, and draft/run the focused regression probe. I verified the failure and all reported commands against the exact head above.
|
Thanks for the detailed review, @nrps9909. I reproduced the stale eligibility case with stable The dependency was intentionally omitted following @zombieJ's earlier performance feedback, since inline callbacks could otherwise trigger a full-data scan on every parent render. @zombieJ, could you confirm which trade-off you prefer here? The direct correctness fix would be to restore I also noticed that My preference is to preserve correctness and let consumers stabilize equivalent predicates with |
Summary
Add an opt-in expand/collapse-all control to the expandable column header.
This is intended to support the use case described in ant-design/ant-design#8791.
The implementation is ready for maintainer review. The corresponding Ant Design integration will follow once the API is accepted and released.
API
expandable.showExpandAllto enable the header control.expandable.onExpandAllto observe expand/collapse-all actions.components.ExpandIconfor both row expansion and the expand-all control.expandable.expandIcon, retaining it as a row-only fallback for backward compatibility.expandable.columnTitleto use object-based render props, exposing the generated expand-all control asexpandIcon.components.ExpandIconreceives:type: 'row' | 'all'expandedexpandableonClickprefixClsrecordfor row controls onlyexpandable.columnTitlefollows the object-based render props convention used by Ant Design column titles:Static
ReactNodetitles keep their existing behavior.Behavior
rowExpandableare affected.expandedRowKeysare supported.components.ExpandIcontakes precedence over the deprecatedexpandable.expandIconfallback.expandedRowRenderand nested/tree row controls.expandedRowRendertables.expandable.columnTitlereceives{ expandIcon }when used as a render function.Summary by CodeRabbit
新功能
expandIcon标记为弃用，建议使用components.ExpandIcon。文档
测试