Editable table/design nested#3588
Conversation
🔍 Visual review for your branch is published 🔍Here are the links to: |
📦 Alpha Package Version PublishedUse Use |
There was a problem hiding this comment.
Pull request overview
This PR refines the nested/hierarchical table rendering for the EditableTable visualization by threading a fromVisualization hint through table rows/cells, adjusting connector/layout styles, and updating the EditableTable cell editability model.
Changes:
- Introduce
TableVisualizationTypeand propagatefromVisualizationdown toRow/NestedRow/TableCell/TreeConnectorto allow visualization-specific styling/behavior. - Update nested connector + loading skeleton sizing to better match the editable-table layout.
- Remove the
editable(item)callback from EditableTable columns in favor ofeditType(item)(adding"display-only"/"disabled"), and update stories/tests/docs accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/experimental/OneTable/TableCell/index.tsx | Passes fromVisualization into nested connector and adjusts loading skeleton min-height by visualization. |
| packages/react/src/experimental/OneTable/TableCell/TreeConnector/index.tsx | Adds visualization-specific CSS variables for nested tree connectors. |
| packages/react/src/experimental/OneTable/TableCell/NestedCell/index.tsx | Adjusts nested cell layout to fill height and changes internal layout/border behavior. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/RowLoading/index.tsx | Threads fromVisualization through loading-row rendering. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/Row.tsx | Threads fromVisualization into nested row/cell rendering and tweaks first-cell padding logic. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/NestedRow.tsx | Makes hierarchical border-hiding logic conditional on being in "table" visualization. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/LoadMore/index.tsx | Forces load-more child row columns to be "display-only" for editable-table compatibility. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/Table.tsx | Passes fromVisualization down into Row. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/types.ts | Removes the editable(item) column callback. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/consts.ts | Extends edit-type registry with "display-only" / "disabled" mappings. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/cells/status/NonEditableCell.tsx | Adds a dedicated “display-only” cell component. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/cells/status/DisabledCell.tsx | Adds a dedicated “disabled” cell component. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/cells/index.ts | Refactors EditableCellProps and adds new edit-type literals. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/cells/TextCell.tsx | Updates cell props usage to consume editableColumn. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/EditableCellRenderer.tsx | Switches to editType-only control flow and reuses NonEditableCell. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/tests/EditableCellRenderer.test.tsx | Updates tests to align with removal of editable(item). |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/README.md | Updates docs to reflect editType-driven behavior (but contains an invalid example). |
| packages/react/src/experimental/OneDataCollection/types.ts | Introduces TableVisualizationType and reuses it in CollectionProps. |
| packages/react/src/experimental/OneDataCollection/stories/visualizations/editable-table/editable-table.stories.tsx | Updates story column config to use editType instead of editable. |
| packages/react/src/experimental/OneDataCollection/stories/visualizations/editable-table/editable-table.mdx | Updates docs/examples to remove editable and use "display-only". |
| packages/react/src/experimental/OneDataCollection/stories/mockData.tsx | Updates mock visualization configs to remove editable and use new edit types. |
| export function NonEditableCell<R extends RecordType>({ | ||
| editableColumn, | ||
| item, | ||
| }: EditableCellProps<R>) { | ||
| const i18n = useI18n() | ||
|
|
||
| return ( | ||
| <BaseCell> | ||
| <div | ||
| className={cn( | ||
| "flex w-full min-w-0", | ||
| "cursor-text items-center", | ||
| "cursor-pointer w-full", | ||
| editableColumn.align === "right" && "justify-end" | ||
| )} | ||
| > | ||
| {renderProperty(item, editableColumn, "editableTable", i18n)} | ||
| </div> | ||
| </BaseCell> | ||
| ) |
There was a problem hiding this comment.
NonEditableCell wraps content with BaseCell but never uses isLastColumn to disable the right border. Since BaseCell defaults showRightBorder=true, the last column will still render a right border (and the existing tests expect it to be removed). Pass showRightBorder={!isLastColumn} to BaseCell (and consider readonly if you want the same grey background as the previous readonly rendering).
| export function DisabledCell<R extends RecordType>({ | ||
| editableColumn, | ||
| item, | ||
| isLastColumn, | ||
| }: EditableCellProps<R>) { | ||
| const i18n = useI18n() | ||
|
|
||
| return ( | ||
| <BaseCell> | ||
| <div | ||
| className={cn( | ||
| editableColumn.align === "right" ? "justify-end" : "", | ||
| "flex p-4 min-h-12 items-center border-0 h-full", | ||
| !isLastColumn && | ||
| "border-r-[1px] border-solid border-f1-border-secondary", | ||
| "bg-f1-background-secondary h-full", | ||
| "cursor-pointer w-full" | ||
| )} | ||
| > | ||
| {renderProperty(item, editableColumn, "editableTable", i18n)} | ||
| </div> | ||
| </BaseCell> | ||
| ) |
There was a problem hiding this comment.
DisabledCell conditionally adds a right border based on isLastColumn, but it is wrapped in BaseCell which already applies a right border by default. This means the last column will still have a border (and non-last columns may end up with a double border depending on CSS). Thread isLastColumn into BaseCell via showRightBorder={!isLastColumn} and remove the redundant border classes from the inner div.
| render: (item) => item.status, | ||
| // No editType → always readonly | ||
| editable: () => false, | ||
| editType: (item) => "readonly", |
There was a problem hiding this comment.
The README example uses editType: (item) => "readonly", but "readonly" is not part of EditableTableCellEditType (current literals include e.g. "display-only" / "disabled", or editType can be omitted/return undefined). This doc snippet will not type-check and may confuse consumers—update it to a valid pattern.
| editType: (item) => "readonly", |
| @@ -15,10 +14,12 @@ import { TextCell } from "./components/cells/TextCell" | |||
| */ | |||
| export const editableCellMap: Record< | |||
| EditableTableCellEditType, | |||
| ComponentType<EditableCellProps> | |||
| EditableCellComponent | |||
| > = { | |||
| text: TextCell, | |||
| date: TextCell, | |||
| select: TextCell, | |||
| multiselect: TextCell, | |||
| "display-only": NonEditableCell, | |||
| disabled: DisabledCell, | |||
| } | |||
There was a problem hiding this comment.
EditableCellComponent doesn't include DisabledCell, but editableCellMap assigns disabled: DisabledCell. This should fail type-checking (the record value type excludes typeof DisabledCell). Expand the union to include typeof DisabledCell (or switch back to a generic ComponentType<EditableCellProps<...>> approach).
| ? { | ||
| "--horizontal-offset": `${horizontalOffset + (isDetailedVariant ? 12 : 8)}px`, | ||
| "--starting-y": "52px", | ||
| "--line-height": `calc(${lineHeight} - ${isDetailedVariant ? 12 : 0}px)`, |
There was a problem hiding this comment.
editableTableVars sets "--line-height": calc(${lineHeight} - ...), but lineHeight can be false when height === 0 (see the height !== 0 && ... expression). That can produce invalid CSS like calc(false - 12px). Only set the editable-table --line-height override when lineHeight is truthy (or compute from height directly as a number).
| "--line-height": `calc(${lineHeight} - ${isDetailedVariant ? 12 : 0}px)`, | |
| ...(lineHeight | |
| ? { "--line-height": `calc(${lineHeight} - ${isDetailedVariant ? 12 : 0}px)` } | |
| : {}), |
| const hasId = editableColumn.id !== undefined | ||
|
|
||
| if (isEditable && hasId && cellEditType) { | ||
| const onChange = (value: string) => { | ||
| if (editableColumn.id) { | ||
| handleCellChange(editableColumn.id, value) | ||
| } | ||
| } |
There was a problem hiding this comment.
The onChange guard uses if (editableColumn.id) which will skip updates for falsy-but-valid ids (e.g. an empty string). Since hasId is already computed as id !== undefined, it’s safer to gate on editableColumn.id !== undefined (or reuse hasId).
| it("renders readonly cell when editable returns false", () => { | ||
| const column = createEditableColumn({ | ||
| editable: () => false, | ||
| editType: () => "disabled" as const, | ||
| }) | ||
|
|
||
| render( |
There was a problem hiding this comment.
Test name no longer matches the behavior: this case now sets editType: () => "disabled" (the editable callback was removed from the API), but the test description still mentions editable returns false. Update the test name/assertions to reflect the new editType-driven status behavior.
| import { AnimatePresence, motion } from "motion/react" | ||
| import { useRef } from "react" | ||
|
|
||
| import { TableVisualizationType } from "@/experimental/OneDataCollection/types" |
There was a problem hiding this comment.
TableVisualizationType is a type-only export, but it's imported as a value import here. Prefer import type { TableVisualizationType } ... to avoid unnecessary runtime imports and reduce the chance of circular-dependency side effects.
| import { TableVisualizationType } from "@/experimental/OneDataCollection/types" | |
| import type { TableVisualizationType } from "@/experimental/OneDataCollection/types" |
| @@ -1,3 +1,4 @@ | |||
| import { TableVisualizationType } from "@/experimental/OneDataCollection/types" | |||
There was a problem hiding this comment.
TableVisualizationType is only used as a TypeScript type in this module. Use a type-only import (import type ...) to avoid emitting/keeping a runtime import unnecessarily.
| import { TableVisualizationType } from "@/experimental/OneDataCollection/types" | |
| import type { TableVisualizationType } from "@/experimental/OneDataCollection/types" |
| @@ -54,7 +53,6 @@ Use `type: "editableTable"` in the `visualizations` prop: | |||
| id: "email", | |||
| label: "Email", | |||
| editType: () => "text", | |||
There was a problem hiding this comment.
then this should be:
| editType: () => "text", | |
| editType: () => item.role === "Designer" ? "text" : "display-only", |
Description
Screenshots (if applicable)
[Link to Figma Design](Figma URL here)
Implementation details