-
Notifications
You must be signed in to change notification settings - Fork 629
[refactor] Give table consumers class hooks that outlive antd #5960
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
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 |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import {AVT, stampTableDom, toAntdColumns} from "@agenta/ui/table" | ||
| import {describe, expect, it} from "vitest" | ||
|
|
||
| /** | ||
| * The table's stable class hooks. App code targets `avt-*` so a selector does not depend on | ||
| * antd's DOM, which the render-leaf swap will replace. If these break, consumer styling | ||
| * silently stops applying, so the contract is pinned here rather than left to a browser pass. | ||
| */ | ||
|
|
||
| interface FakeNode { | ||
| classes: Set<string> | ||
| classList: {add: (c: string) => void} | ||
| } | ||
|
|
||
| const node = (): FakeNode => { | ||
| const classes = new Set<string>() | ||
| return {classes, classList: {add: (c: string) => classes.add(c)}} | ||
| } | ||
|
|
||
| /** Minimal stand-in for the mounted table: querySelector over a fixed selector map. */ | ||
| const container = (found: Record<string, FakeNode>) => | ||
| ({ | ||
| querySelector: (selector: string) => found[selector] ?? null, | ||
| }) as unknown as HTMLElement | ||
|
|
||
| describe("stampTableDom", () => { | ||
| it("stamps the structural hooks onto antd's nodes", () => { | ||
| const nodes = { | ||
| ".ant-table-container": node(), | ||
| ".ant-table-body": node(), | ||
| ".ant-table-thead": node(), | ||
| } | ||
| stampTableDom(container(nodes)) | ||
|
|
||
| expect([...nodes[".ant-table-container"].classes]).toEqual([AVT.container]) | ||
| expect([...nodes[".ant-table-body"].classes]).toEqual([AVT.body]) | ||
| expect([...nodes[".ant-table-thead"].classes]).toEqual([AVT.header]) | ||
| }) | ||
|
|
||
| it("skips nodes that are not present rather than throwing", () => { | ||
| expect(() => stampTableDom(container({}))).not.toThrow() | ||
| expect(() => stampTableDom(null)).not.toThrow() | ||
| }) | ||
| }) | ||
|
|
||
| describe("toAntdColumns cell hooks", () => { | ||
| interface Row { | ||
| id: string | ||
| } | ||
|
|
||
| it("adds the cell hooks to a plain column", () => { | ||
| const [column] = toAntdColumns<Row>([{key: "id", title: "ID"}]) | ||
|
|
||
| expect(column.onCell?.({id: "a"}, 0)).toEqual({className: AVT.cell}) | ||
| expect(column.onHeaderCell?.(column, 0)).toEqual({className: AVT.headerCell}) | ||
| }) | ||
|
|
||
| it("keeps a column's own cell props and appends the hook", () => { | ||
| const [column] = toAntdColumns<Row>([ | ||
| { | ||
| key: "id", | ||
| onCell: () => ({className: "mine", colSpan: 2}), | ||
| }, | ||
| ]) | ||
|
|
||
| expect(column.onCell?.({id: "a"}, 0)).toEqual({ | ||
| className: `mine ${AVT.cell}`, | ||
| colSpan: 2, | ||
| }) | ||
| }) | ||
|
|
||
| it("reaches columns nested in a group", () => { | ||
| const [group] = toAntdColumns<Row>([ | ||
| {key: "g", title: "Group", children: [{key: "id", title: "ID"}]}, | ||
| ]) | ||
| const child = (group as {children: (typeof group)[]}).children[0] | ||
|
|
||
| expect(child.onCell?.({id: "a"}, 0)).toEqual({className: AVT.cell}) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -137,6 +137,7 @@ export type { | |||||
| ColumnSorterConfig, | ||||||
| } from "./columnDef" | ||||||
| export {toAntdColumns, fromAntdColumns} from "./antdColumns" | ||||||
| export {AVT, ANTD_SELECTOR, stampTableDom, type AvtClass} from "./tableDom" | ||||||
|
Contributor
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. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Keep Line 140 exposes Ant Design DOM selectors as a public API. Proposed fix-export {AVT, ANTD_SELECTOR, stampTableDom, type AvtClass} from "./tableDom"
+export {AVT, stampTableDom, type AvtClass} from "./tableDom"📝 Committable suggestion
Suggested change
|
||||||
| export type {VisibilityRegistrationHandler} from "./components/ColumnVisibilityHeader" | ||||||
|
|
||||||
| // Shared hooks for cell renderers | ||||||
|
|
||||||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: Agenta-AI/agenta
Length of output: 22377
🏁 Script executed:
Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
Repository: Agenta-AI/agenta
Length of output: 50374
🏁 Script executed:
Repository: Agenta-AI/agenta
Length of output: 350
🌐 Web query:
React official documentation useEffect after paint useLayoutEffect before browser repaint server rendering warning💡 Result:
The fundamental difference between useEffect and useLayoutEffect lies in their execution timing relative to the browser's painting process [1][2][3]. useEffect: - Runs asynchronously after the browser has painted the updated screen [1][2]. - It is the default choice for most side effects (e.g., data fetching, subscriptions, setting up event handlers) because it avoids blocking the browser from updating the user interface [1][4][3]. useLayoutEffect: - Runs synchronously after React performs all DOM mutations, but before the browser paints those changes to the screen [1][2][3]. - It effectively blocks the browser from repainting until the effect completes [1][2]. - It is intended for specific use cases, such as measuring DOM elements or performing visual mutations that must occur before the user sees the rendered output to prevent visible "flicker" [1][2][3]. Server Rendering Warning: React issues a warning when useLayoutEffect is used during server-side rendering because this hook cannot execute on the server [5][6]. On the server, React renders components to a string or stream without a DOM environment; therefore, the code inside useLayoutEffect is skipped [5][7]. The warning serves as a safeguard because useLayoutEffect is typically used to fix visual inconsistencies or measure layout [8][6]. If a component relies on this effect for correct rendering, the server-rendered HTML sent to the client will likely look incorrect or "broken" until the client-side JavaScript loads and the effect eventually runs [6][7]. To resolve this warning: 1. Prefer useEffect: If the logic does not require immediate, blocking layout adjustments, switch to useEffect [1][5]. 2. Conditional Rendering: If a component depends on client-only layout logic, render it conditionally so it only appears after the component has mounted on the client [8][5]. This can be achieved by using state (e.g., initialized to false) that is set to true within a useEffect hook [8][5]. This prevents the "broken" UI from being displayed in the initial server-rendered HTML [6][7].
Citations:
🏁 Script executed:
Repository: Agenta-AI/agenta
Length of output: 26811
Stamp structural hooks before the first paint.
useEffectcan let the first client frame render withoutavt-container,avt-body, oravt-thead, so consumer selectors may not apply. Use a client/server-safe layout effect:useLayoutEffecton the client anduseEffectduring SSR.