noob-components/.trellis/workspace/hechang27-sprt/04-10-code-review.md

Code review

Found 4 urgent issues need to be fixed:

1 Dimension props regressed, so height/maxHeight no longer follow the old contract

FilePath: packages/base/data/list-table-v2/list-table-v2.vue:291 line 291

if (props.height !== undefined) { const h = String(props.height); style.maxHeight = h.endsWith("px") ? h : ${h}px; } // props.maxHeight handling is commented out

height is being applied as maxHeight, maxHeight is ignored entirely, and minHeight is unused. The old component and the demo page both rely on these props behaving distinctly, so PR2 currently breaks the documented sizing behavior.

Suggested fix

Restore the old container contract: set style.height from props.height, apply style.maxHeight from props.maxHeight, honor props.minHeight, and keep viewportHeight derived from the actual constrained container size.

———

2 rowKey is exposed but never used, so virtual rows are keyed by visible index

FilePath: packages/base/data/list-table-v2/list-table-v2.vue:32 line 32

This throws away stable row identity. On page changes, filtering, or reordered data, Vue will reuse row subtrees by index instead of by record identity, which is exactly what rowKey is supposed to prevent.

Suggested fix

Derive a stable key from the actual row, for example getRow(row.index)?.[props.rowKey] ?? row.index, and use that for the row wrapper.

———

3 Pretext width calculation is measuring different text/fonts than the component actually renders

FilePath: packages/base/data/list-table-v2/usePretextColumnWidths.ts:198 line 198

const font = options?.font ?? DEFAULT_FONT; const headerFont = options?.headerFont ?? DEFAULT_HEADER_FONT; ... const headerText = col.name || col.i18n || colKey;

The component renders body text with props.font and now defaults to 14px Microsoft YaHei, but list-table-v2.vue still calls this hook with hardcoded Inter fonts. On top of that, header sizing uses the raw i18n key instead of the translated label. That means the measured widths diverge from the rendered widths, which directly undermines PR2’s “column widths adapt to content” acceptance criterion.

Suggested fix

Pass the real rendered font into usePretextColumnWidths from packages/base/data/list-table-v2/list-table-v2.vue:241, derive the header font from that same base font, and measure the translated header text instead of col.i18n.

———

4 The rewrite dropped timestampFormat from the public API

FilePath: packages/base/data/list-table-v2/list-table-v2.vue:110 line 110

interface Props { ... border?: boolean; rowHeight?: number; estimatedRowHeight?: number; headerHeight?: number; font?: string; debug?: boolean; }

The old component exposed timestampFormat, but the new runtime props and exported ListTableProps no longer include it, while timestamp rendering is still part of the component contract. That is a backward-compatibility break against the PRD goal of keeping the existing prop interface.

Suggested fix

Re-add timestampFormat to both the runtime props and packages/base/data/list-table-v2/types.ts:87, then thread it into the timestamp formatting path so existing consumers keep their display override.

———

Would you like me to use the Suggested fix section to address these issues?

Response

› 4 is intentional, 1/2/3 should be fixed. However, there is an additional problem: when using custom slots like the table 11 in the demo page, the inner elements consistently overflows the table cell even with overflow: hidden...