-
-
Notifications
You must be signed in to change notification settings - Fork 618
fix: table onCell-method cause offset in width calculation #1315
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,6 +8,8 @@ import type { ColumnType, CustomizeComponent } from '../interface'; | |||||||||||||||||||||||||
import ExpandedRow from './ExpandedRow'; | ||||||||||||||||||||||||||
import { computedExpandedClassName } from '../utils/expandUtil'; | ||||||||||||||||||||||||||
import type { TableProps } from '..'; | ||||||||||||||||||||||||||
import useStickyOffsets from '../hooks/useStickyOffsets'; | ||||||||||||||||||||||||||
import { getCellFixedInfo } from '../utils/fixUtil'; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
export interface BodyRowProps<RecordType> { | ||||||||||||||||||||||||||
record: RecordType; | ||||||||||||||||||||||||||
|
@@ -43,12 +45,16 @@ export function getCellProps<RecordType>( | |||||||||||||||||||||||||
index: number, | ||||||||||||||||||||||||||
rowKeys: React.Key[] = [], | ||||||||||||||||||||||||||
expandedRowOffset = 0, | ||||||||||||||||||||||||||
rowStickyOffsets?: ReturnType<typeof useStickyOffsets>, | ||||||||||||||||||||||||||
hasColSpanZero?: boolean, | ||||||||||||||||||||||||||
cachedCellProps?: Record<string, any>, | ||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||
const { | ||||||||||||||||||||||||||
record, | ||||||||||||||||||||||||||
prefixCls, | ||||||||||||||||||||||||||
columnsKey, | ||||||||||||||||||||||||||
fixedInfoList, | ||||||||||||||||||||||||||
flattenColumns, | ||||||||||||||||||||||||||
expandIconColumnIndex, | ||||||||||||||||||||||||||
nestExpandable, | ||||||||||||||||||||||||||
indentSize, | ||||||||||||||||||||||||||
|
@@ -61,9 +67,11 @@ export function getCellProps<RecordType>( | |||||||||||||||||||||||||
} = rowInfo; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const key = columnsKey[colIndex]; | ||||||||||||||||||||||||||
const fixedInfo = fixedInfoList[colIndex]; | ||||||||||||||||||||||||||
let fixedInfo = fixedInfoList[colIndex]; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// ============= Used for nest expandable ============= | ||||||||||||||||||||||||||
if (column.fixed && hasColSpanZero && rowStickyOffsets) { | ||||||||||||||||||||||||||
fixedInfo = getCellFixedInfo(colIndex, colIndex, flattenColumns, rowStickyOffsets); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
let appendCellNode: React.ReactNode; | ||||||||||||||||||||||||||
if (colIndex === (expandIconColumnIndex || 0) && nestExpandable) { | ||||||||||||||||||||||||||
appendCellNode = ( | ||||||||||||||||||||||||||
|
@@ -83,7 +91,7 @@ export function getCellProps<RecordType>( | |||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const additionalCellProps = column.onCell?.(record, index) || {}; | ||||||||||||||||||||||||||
const additionalCellProps = { ...(cachedCellProps || column.onCell?.(record, index) || {}) }; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Expandable row has offset | ||||||||||||||||||||||||||
if (expandedRowOffset) { | ||||||||||||||||||||||||||
|
@@ -143,6 +151,7 @@ function BodyRow<RecordType extends { children?: readonly RecordType[] }>( | |||||||||||||||||||||||||
const { | ||||||||||||||||||||||||||
prefixCls, | ||||||||||||||||||||||||||
flattenColumns, | ||||||||||||||||||||||||||
colWidths, | ||||||||||||||||||||||||||
expandedRowClassName, | ||||||||||||||||||||||||||
expandedRowRender, | ||||||||||||||||||||||||||
rowProps, | ||||||||||||||||||||||||||
|
@@ -152,6 +161,20 @@ function BodyRow<RecordType extends { children?: readonly RecordType[] }>( | |||||||||||||||||||||||||
rowSupportExpand, | ||||||||||||||||||||||||||
} = rowInfo; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const cellPropsCache = React.useMemo(() => { | ||||||||||||||||||||||||||
return flattenColumns.map(col => col.onCell?.(record, index) || {}); | ||||||||||||||||||||||||||
}, [flattenColumns, record, index]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const hasColSpanZero = React.useMemo(() => { | ||||||||||||||||||||||||||
return cellPropsCache.some(cellProps => (cellProps.colSpan ?? 1) === 0); | ||||||||||||||||||||||||||
}, [cellPropsCache]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const rowStickyOffsets = useStickyOffsets( | ||||||||||||||||||||||||||
colWidths, | ||||||||||||||||||||||||||
flattenColumns, | ||||||||||||||||||||||||||
hasColSpanZero ? { record, rowIndex: index } : undefined, | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
Comment on lines
+172
to
+176
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rowStickyOffsets hook is called on every render even when hasColSpanZero is false. Consider conditionally calling this hook only when needed to avoid unnecessary calculations.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 条件性调用 Hook违反规则 |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Force render expand row if expanded before | ||||||||||||||||||||||||||
const expandedRef = React.useRef(false); | ||||||||||||||||||||||||||
expandedRef.current ||= expanded; | ||||||||||||||||||||||||||
|
@@ -196,6 +219,9 @@ function BodyRow<RecordType extends { children?: readonly RecordType[] }>( | |||||||||||||||||||||||||
index, | ||||||||||||||||||||||||||
rowKeys, | ||||||||||||||||||||||||||
expandedRowInfo?.offset, | ||||||||||||||||||||||||||
rowStickyOffsets, | ||||||||||||||||||||||||||
hasColSpanZero, | ||||||||||||||||||||||||||
cellPropsCache[colIndex], | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -336,4 +336,139 @@ describe('Table.FixedColumn', () => { | |||||||||||||||||||||||||||
expect(container.querySelector('.rc-table')).toHaveClass('rc-table-fix-start-shadow-show'); | ||||||||||||||||||||||||||||
expect(container.querySelector('.rc-table')).toHaveClass('rc-table-fix-end-shadow-show'); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
describe('colSpan=0 with fixed columns regression test', () => { | ||||||||||||||||||||||||||||
interface TestDataType { | ||||||||||||||||||||||||||||
key: string; | ||||||||||||||||||||||||||||
col0: string; | ||||||||||||||||||||||||||||
col1: string; | ||||||||||||||||||||||||||||
col2: string; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const testColumns: ColumnsType<TestDataType> = [ | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
title: 'Column 0', | ||||||||||||||||||||||||||||
dataIndex: 'col0', | ||||||||||||||||||||||||||||
key: 'col0', | ||||||||||||||||||||||||||||
width: 100, | ||||||||||||||||||||||||||||
fixed: 'left', | ||||||||||||||||||||||||||||
onCell: (record, index) => { | ||||||||||||||||||||||||||||
if (index === 1) { | ||||||||||||||||||||||||||||
return { colSpan: 0 }; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return {}; | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
title: 'Column 1', | ||||||||||||||||||||||||||||
dataIndex: 'col1', | ||||||||||||||||||||||||||||
key: 'col1', | ||||||||||||||||||||||||||||
width: 120, | ||||||||||||||||||||||||||||
fixed: 'left', | ||||||||||||||||||||||||||||
onCell: (record, index) => { | ||||||||||||||||||||||||||||
if (index === 1) { | ||||||||||||||||||||||||||||
return { colSpan: 2 }; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return {}; | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
title: 'Column 2', | ||||||||||||||||||||||||||||
dataIndex: 'col2', | ||||||||||||||||||||||||||||
key: 'col2', | ||||||||||||||||||||||||||||
width: 150, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const testData: TestDataType[] = [ | ||||||||||||||||||||||||||||
{ key: '0', col0: 'Row0-Col0', col1: 'Row0-Col1', col2: 'Row0-Col2' }, | ||||||||||||||||||||||||||||
{ key: '1', col0: 'Row1-Col0', col1: 'Row1-Merged', col2: 'Row1-Col2' }, | ||||||||||||||||||||||||||||
{ key: '2', col0: 'Row2-Col0', col1: 'Row2-Col1', col2: 'Row2-Col2' }, | ||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
it('should calculate correct sticky offsets when colSpan=0 exists', async () => { | ||||||||||||||||||||||||||||
const { container } = render( | ||||||||||||||||||||||||||||
<Table columns={testColumns} data={testData} scroll={{ x: 500 }} />, | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
await triggerResize(container.querySelector<HTMLElement>('.rc-table')); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
act(() => { | ||||||||||||||||||||||||||||
const coll = container.querySelector('.rc-table-resize-collection'); | ||||||||||||||||||||||||||||
if (coll) { | ||||||||||||||||||||||||||||
triggerResize(coll as HTMLElement); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
await act(async () => { | ||||||||||||||||||||||||||||
vi.runAllTimers(); | ||||||||||||||||||||||||||||
await Promise.resolve(); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const rows = container.querySelectorAll('.rc-table-tbody .rc-table-row'); | ||||||||||||||||||||||||||||
expect(rows).toHaveLength(3); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const secondRow = rows[1]; | ||||||||||||||||||||||||||||
const cells = secondRow.querySelectorAll('.rc-table-cell'); | ||||||||||||||||||||||||||||
expect(cells).toHaveLength(2); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const mergedCell = cells[0]; | ||||||||||||||||||||||||||||
expect(mergedCell).toHaveAttribute('colSpan', '2'); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
expect(mergedCell.textContent).toContain('Row1-Merged'); | ||||||||||||||||||||||||||||
const hasFixedLeftClass = mergedCell.classList.contains('rc-table-cell-fix-left'); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if (hasFixedLeftClass) { | ||||||||||||||||||||||||||||
const cellStyle = window.getComputedStyle(mergedCell); | ||||||||||||||||||||||||||||
expect(cellStyle.left).toBe('0px'); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
+419
to
+424
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 让断言更强:固定列类名应为必然条件,且使用逻辑方向属性以避免环境差异 当前以“如果包含 rc-table-cell-fix-left 再断言 left”为条件,可能在类名缺失时静默通过,掩盖回归。并且使用
[建议变更如下] - const hasFixedLeftClass = mergedCell.classList.contains('rc-table-cell-fix-left');
-
- if (hasFixedLeftClass) {
- const cellStyle = window.getComputedStyle(mergedCell);
- expect(cellStyle.left).toBe('0px');
- }
+ // 固定左列应当存在
+ expect(mergedCell).toHaveClass('rc-table-cell-fix-left');
+ // 兼容传统属性与逻辑方向属性
+ const cellStyle = window.getComputedStyle(mergedCell);
+ const left = cellStyle.left;
+ const insetInlineStart = cellStyle.getPropertyValue('inset-inline-start');
+ expect(left === '0px' || insetInlineStart === '0px').toBeTruthy(); 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
it('should work correctly with expandable rows', async () => { | ||||||||||||||||||||||||||||
const expandableTestData = testData.map(item => ({ | ||||||||||||||||||||||||||||
...item, | ||||||||||||||||||||||||||||
children: | ||||||||||||||||||||||||||||
item.key === '1' | ||||||||||||||||||||||||||||
? [{ key: '1-0', col0: 'Child-Col0', col1: 'Child-Col1', col2: 'Child-Col2' }] | ||||||||||||||||||||||||||||
: undefined, | ||||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { container } = render( | ||||||||||||||||||||||||||||
<Table | ||||||||||||||||||||||||||||
columns={testColumns} | ||||||||||||||||||||||||||||
data={expandableTestData} | ||||||||||||||||||||||||||||
scroll={{ x: 500 }} | ||||||||||||||||||||||||||||
expandable={{ | ||||||||||||||||||||||||||||
expandedRowKeys: ['1'], | ||||||||||||||||||||||||||||
expandRowByClick: true, | ||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||
/>, | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
await triggerResize(container.querySelector<HTMLElement>('.rc-table')); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
act(() => { | ||||||||||||||||||||||||||||
const coll = container.querySelector('.rc-table-resize-collection'); | ||||||||||||||||||||||||||||
if (coll) { | ||||||||||||||||||||||||||||
triggerResize(coll as HTMLElement); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
await act(async () => { | ||||||||||||||||||||||||||||
vi.runAllTimers(); | ||||||||||||||||||||||||||||
await Promise.resolve(); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const allRows = container.querySelectorAll('.rc-table-tbody .rc-table-row'); | ||||||||||||||||||||||||||||
expect(allRows.length).toBeGreaterThan(3); // 包含展开的子行 | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const parentRow = allRows[1]; | ||||||||||||||||||||||||||||
const parentCells = parentRow.querySelectorAll('.rc-table-cell'); | ||||||||||||||||||||||||||||
expect(parentCells).toHaveLength(2); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const childRow = allRows[2]; | ||||||||||||||||||||||||||||
const childCells = childRow.querySelectorAll('.rc-table-cell'); | ||||||||||||||||||||||||||||
expect(childCells).toHaveLength(3); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
}); |
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.
The cellPropsCache is recalculated on every render when record or index changes. For large tables with many columns, this could be expensive. Consider memoizing individual cell props or using a more granular dependency array.
Copilot uses AI. Check for mistakes.