Conversation
RafaMellado
force-pushed
the
editable-table/design
branch
from
28e6bdd to
e010849
Compare
There was a problem hiding this comment.
Pull request overview
Adds styling and plumbing needed for “editable tables” UI, including a transparent input mode and per-cell rendering behavior to better match the table design.
Changes:
- Thread a new
transparentprop throughInput→InputFieldand use it in editable table cells. - Extend table
cellRendererto receiveisLastColumnand adjust cell padding/borders accordingly. - Introduce
BaseCellwrapper for editable table cell layout/borders.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/ui/input.tsx | Passes transparent through Input and updates prop typing. |
| packages/react/src/ui/InputField/InputField.tsx | Adds transparent prop and conditional class logic for transparent rendering. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/Row.tsx | Extends cellRenderer to receive isLastColumn and tweaks row/cell classes when rendering custom cells. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/types.ts | Import order/layout change. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/cells/index.ts | Minor formatting change. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/cells/TextCell.tsx | Wraps text cell with BaseCell and uses transparent Input. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/cells/BaseCell.tsx | Adds a base wrapper providing consistent cell sizing/borders. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/EditableCellRenderer.tsx | Accepts isLastColumn and applies right border conditionally. |
| packages/react/src/experimental/Forms/Fields/Input/internal.tsx | Prop typing updated to include transparent. |
| packages/react/src/experimental/Forms/Fields/Input/F0Input.tsx | Adds className to exported input props. |
| | "transparent" | ||
| > |
There was a problem hiding this comment.
"transparent" is added to the Omit list for InputProps, but the component then destructures and forwards transparent. This makes transparent not part of InputProps and will cause a TypeScript error / mismatch. Remove "transparent" from the Omit list, or explicitly add transparent?: boolean back onto InputProps after the Omit.
| | "transparent" | |
| > | |
| > & { | |
| transparent?: boolean | |
| } |
| onKeyDown, | ||
| readonly, | ||
| buttonToggle, | ||
| transparent, |
There was a problem hiding this comment.
"transparent" is added to the Omit list for InputProps, but the component then destructures and forwards transparent. This makes transparent not part of InputProps and will cause a TypeScript error / mismatch. Remove "transparent" from the Omit list, or explicitly add transparent?: boolean back onto InputProps after the Omit.
| inputRef={ref} | ||
| readonly={readonly} | ||
| buttonToggle={buttonToggle} | ||
| transparent={transparent} |
There was a problem hiding this comment.
"transparent" is added to the Omit list for InputProps, but the component then destructures and forwards transparent. This makes transparent not part of InputProps and will cause a TypeScript error / mismatch. Remove "transparent" from the Omit list, or explicitly add transparent?: boolean back onto InputProps after the Omit.
There was a problem hiding this comment.
Props are destructured but in the component below
| @@ -395,7 +397,7 @@ const InputField = forwardRef<HTMLDivElement, InputFieldProps<string>>( | |||
| className={cn( | |||
| "flex flex-col gap-2", | |||
| "pointer-events-none", | |||
There was a problem hiding this comment.
This removes the previous disabled && "cursor-not-allowed" class entirely (it was replaced by the transparent styling). Disabled inputs likely no longer show the not-allowed cursor. Keep the disabled cursor class and add transparent styles in addition (e.g., include both conditions).
| "pointer-events-none", | |
| "pointer-events-none", | |
| disabled && "cursor-not-allowed", |
| disableHover && "hover:bg-transparent", | ||
| isSelected && "bg-f1-background-selected-secondary" | ||
| isSelected && "bg-f1-background-selected-secondary", | ||
| CellRenderer && "bg-f1-background-secondary" |
There was a problem hiding this comment.
When CellRenderer is present, the row gets bg-f1-background-secondary after the selected background class, which can override the selected styling due to class order. Consider moving the CellRenderer background before isSelected (or making it conditional on !isSelected) so selection remains visually correct.
| CellRenderer && "bg-f1-background-secondary" | |
| CellRenderer && !isSelected && "bg-f1-background-secondary" |
| rowWithChildren, | ||
| tableWithChildren, | ||
| }} | ||
| {...(CellRenderer ? { className: "p-0 align-middle" } : {})} |
There was a problem hiding this comment.
Setting className via a conditional spread can overwrite any existing className coming from earlier props (e.g., column/cell props) and is easy to break as attributes change. Prefer computing a merged className with cn(...) and passing it explicitly so existing cell-level classes are preserved.
| {...(CellRenderer ? { className: "p-0 align-middle" } : {})} | |
| className={CellRenderer ? "p-0 align-middle" : undefined} |
|
|
||
| export function BaseCell({ children }: { children: React.ReactNode }) { |
There was a problem hiding this comment.
This file references React.ReactNode but does not import React types. In a TSX module, React won’t be in scope just for types. Import type { ReactNode } from "react" and use children: ReactNode, or import type * as React from "react".
| export function BaseCell({ children }: { children: React.ReactNode }) { | |
| import type { ReactNode } from "react" | |
| export function BaseCell({ children }: { children: ReactNode }) { |
| isLastColumn, | ||
| }: CellRendererProps<R, Sortings, Summaries> & { isLastColumn: boolean }) { |
There was a problem hiding this comment.
Row’s cellRenderer prop type was updated to accept isLastColumn?: boolean (optional), but EditableCellRenderer now requires isLastColumn: boolean. To keep the renderer usable where isLastColumn isn’t provided, make it optional here (and default it), or align the types so both sides agree.
| isLastColumn, | |
| }: CellRendererProps<R, Sortings, Summaries> & { isLastColumn: boolean }) { | |
| isLastColumn = false, | |
| }: CellRendererProps<R, Sortings, Summaries> & { isLastColumn?: boolean }) { |
|
|
||
| export function BaseCell({ children }: { children: React.ReactNode }) { | ||
| return ( | ||
| <div | ||
| className={cn( | ||
| "flex w-full min-w-0 min-h-12 border-0 border-r-[1px] border-solid border-f1-border-secondary", |
There was a problem hiding this comment.
BaseCell always renders a right border, but the table cell renderer now has isLastColumn to suppress the last-column border. If BaseCell is used in the last column, it will still draw a right border and defeat that intent. Consider letting BaseCell accept an isLastColumn/showRightBorder prop (or removing the border from BaseCell and centralizing border logic in the table cell renderer).
| export function BaseCell({ children }: { children: React.ReactNode }) { | |
| return ( | |
| <div | |
| className={cn( | |
| "flex w-full min-w-0 min-h-12 border-0 border-r-[1px] border-solid border-f1-border-secondary", | |
| import React from "react" | |
| export function BaseCell({ | |
| children, | |
| showRightBorder = true, | |
| }: { | |
| children: React.ReactNode | |
| showRightBorder?: boolean | |
| }) { | |
| return ( | |
| <div | |
| className={cn( | |
| "flex w-full min-w-0 min-h-12 border-0 border-r-[1px] border-solid border-f1-border-secondary", | |
| !showRightBorder && "border-r-0", |
RafaMellado
force-pushed
the
editable-table/design
branch
from
e010849 to
748673b
Compare
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
RafaMellado
force-pushed
the
editable-table/design
branch
from
748673b to
767d327
Compare
🔍 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
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/EditableCellRenderer.tsx:101
- In edit mode,
isLastColumnis used to drop the right border for read-only cells, but editable cells always renderBaseCellwithshowRightBorderdefaulting totrue, so the last editable column will still show a right border. Pass theisLastColumninformation through (e.g. viaEditableCellProps/BaseCell.showRightBorder) or move the border styling to a wrapper that can consistently apply it for both editable and read-only content.
if (isEditable && hasId && cellEditType) {
const CellComponent = editableCellMap[cellEditType]
if (CellComponent) {
const value = getCellValue(localItem, editableColumn)
const error = editableColumn.id
? cellErrors[editableColumn.id]
: undefined
const loading = editableColumn.id
? (cellLoading[editableColumn.id] ?? false)
: false
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions -- stops cell navigation (href/onClick) from firing when interacting with the editor
<div
className="pointer-events-auto"
onClickCapture={(e) => e.stopPropagation()}
onMouseDownCapture={(e) => e.stopPropagation()}
>
<CellComponent
label={editableColumn.label}
value={value}
align={editableColumn.align}
error={error}
loading={loading}
onChange={(v) => {
if (editableColumn.id) {
handleCellChange(editableColumn.id, v)
}
}}
/>
| <CellRenderer item={item} column={column} cellIndex={cellIndex}> | ||
| <CellRenderer | ||
| item={item} | ||
| isLastColumn={cellIndex === columns.length - 1} |
There was a problem hiding this comment.
isLastColumn is calculated only from columns.length, but rows can render an additional Item Actions column after the mapped columns. When item actions are present, the last data column is not actually the last visible column, so renderers relying on isLastColumn (e.g. to remove the right border) will behave incorrectly. Consider incorporating hasItemActions (and any other trailing columns) into the isLastColumn calculation.
| isLastColumn={cellIndex === columns.length - 1} | |
| isLastColumn={!hasItemActions && cellIndex === columns.length - 1} |
| <CellRenderer | ||
| item={item} | ||
| isLastColumn={cellIndex === columns.length - 1} | ||
| column={column} | ||
| cellIndex={cellIndex} | ||
| > |
There was a problem hiding this comment.
Table has existing unit tests for cellRenderer customization, but there is no coverage for the newly-added isLastColumn prop being passed to the renderer (including the edge case where an Item Actions column is present). Adding/adjusting a test in Table/__tests__/Table.spec.tsx would prevent regressions for this new API surface.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| disabled && "cursor-not-allowed bg-f1-background-tertiary", | ||
| inputFieldVariants({ size, canGrow }), | ||
| ], | ||
| transparent && "h-full w-full" |
There was a problem hiding this comment.
When transparent is true, the wrapper drops all focus-within:* ring/border classes (and the input itself doesn't apply focusRing()), which can make keyboard focus indication invisible. Please ensure transparent inputs still have a visible focus style (e.g. apply focusRing()/focus-visible styles to the input or keep a minimal focus ring on the wrapper in transparent mode).
| transparent && "h-full w-full" | |
| transparent && [ | |
| "h-full w-full", | |
| "relative transition-all group", | |
| "focus-within:outline-none focus-within:ring-1 focus-within:ring-offset-1 focus-within:ring-f1-border-hover", | |
| "active-within:ring-1 active-within:ring-f1-border-hover", | |
| ] |
| EditableTableCellEditType, | ||
| ComponentType<EditableCellProps> | ||
| > = { | ||
| text: TextCell, | ||
| date: TextCell, | ||
| select: TextCell, | ||
| multiselect: TextCell, | ||
| } |
There was a problem hiding this comment.
EditableTableCellEditType exposes "date" | "select" | "multiselect", but the registry currently maps all of these to TextCell, which is misleading for consumers and makes it easy to accidentally ship the wrong UI. Consider narrowing the union/registry to only the edit types that are actually implemented (e.g. just "text") until dedicated cells exist, or implement distinct components for the additional types.
| EditableTableCellEditType, | |
| ComponentType<EditableCellProps> | |
| > = { | |
| text: TextCell, | |
| date: TextCell, | |
| select: TextCell, | |
| multiselect: TextCell, | |
| } | |
| Extract<EditableTableCellEditType, "text">, | |
| ComponentType<EditableCellProps> | |
| > = { | |
| text: TextCell, | |
| } |
There was a problem hiding this comment.
Agree, this is going to be modified soon!
| @@ -0,0 +1,24 @@ | |||
| import { ComponentType } from "react" | |||
There was a problem hiding this comment.
ComponentType is only used as a type here; using a type-only import avoids adding an unnecessary runtime import in the emitted JS.
| import { ComponentType } from "react" | |
| import type { ComponentType } from "react" |
| export type InputProps<T extends string> = Omit< | ||
| InputInternalProps<T>, | ||
| (typeof privateProps)[number] | ||
| > | ||
| > & { className?: string } | ||
|
|
There was a problem hiding this comment.
This change adds className to the public props of the (experimental) Input field. In packages/react, the convention is to avoid exposing className on public component APIs (prefer keeping it private via the privateProps pattern) to reduce styling API surface and make future refactors easier.
RafaMellado
force-pushed
the
editable-table/design
branch
2 times, most recently
from
e889455 to
5a71400
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/EditableCellRenderer.tsx:103
isLastColumnis computed and passed down, but it's only used in the non-editable fallback. For editable cells,BaseCellalways applies a right border, so the last column will still render a divider (inconsistent with headers and read-only cells). Consider plumbingisLastColumninto the editable cell (e.g., viaEditableCellProps) and pass it toBaseCellasshowRightBorder={!isLastColumn}(or handle the border in the renderer wrapper).
if (isEditable && hasId && cellEditType) {
const CellComponent = editableCellMap[cellEditType]
if (CellComponent) {
const value = getCellValue(localItem, editableColumn)
const error = editableColumn.id
? cellErrors[editableColumn.id]
: undefined
const loading = editableColumn.id
? (cellLoading[editableColumn.id] ?? false)
: false
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions -- stops cell navigation (href/onClick) from firing when interacting with the editor
<div
className="pointer-events-auto h-full"
onClickCapture={(e) => e.stopPropagation()}
onMouseDownCapture={(e) => e.stopPropagation()}
>
<CellComponent
label={editableColumn.label}
value={value}
align={editableColumn.align}
error={error}
loading={loading}
onChange={(v) => {
if (editableColumn.id) {
handleCellChange(editableColumn.id, v)
}
}}
/>
</div>
)
| @@ -30,11 +30,6 @@ function getCellValue<R extends RecordType>( | |||
| if (typeof rendered === "string") return rendered | |||
| if (typeof rendered === "number") return String(rendered) | |||
|
|
|||
There was a problem hiding this comment.
getCellValue() now silently coerces unsupported render outputs (anything non-string/number) to an empty string. This can make cells appear blank without any signal, which is hard to debug when a column render accidentally returns a renderer definition / ReactNode. Consider restoring a console.warn() in development (or otherwise surfacing an explicit fallback) when a non-primitive is returned.
| if (process.env.NODE_ENV !== "production") { | |
| // In development, warn when a non-primitive value is returned from column.render, | |
| // since getCellValue will coerce it to an empty string and the cell will appear blank. | |
| // This commonly happens if a ReactNode or renderer definition is returned by mistake. | |
| // eslint-disable-next-line no-console -- development-only diagnostic | |
| console.warn( | |
| "[EditableTable] getCellValue received a non-primitive render output.", | |
| { | |
| columnId: column.id, | |
| renderedValue: rendered, | |
| } | |
| ) | |
| } |
| @@ -69,16 +68,8 @@ export function EditableCellRenderer< | |||
|
|
|||
| const hasId = editableColumn.id !== undefined | |||
|
|
|||
There was a problem hiding this comment.
In the editable path, columns with editType but without an id are now silently rendered as read-only (since updates/errors/loading are keyed by id). Consider restoring a development-time warning when isEditable is true but editableColumn.id is missing, so misconfigured columns are caught early.
| if ( | |
| process.env.NODE_ENV !== "production" && | |
| isEditable && | |
| !hasId && | |
| typeof console !== "undefined" | |
| ) { | |
| // In the editable path, columns without an `id` cannot be updated or tracked for | |
| // errors/loading since those are keyed by `id`. Warn in development so that | |
| // misconfigured columns (with `editType`/editable but no `id`) are caught early. | |
| console.warn( | |
| "[EditableTable] Editable column is missing `id` and will be rendered as read-only.", | |
| { | |
| columnLabel: editableColumn.label, | |
| columnKey: editableColumn.key, | |
| } | |
| ) | |
| } |
| error={error} | ||
| loading={loading} |
There was a problem hiding this comment.
TextCell passes error/loading into Input while also enabling transparent. InputField will still render InputMessages for errors, which can expand a table row/cell beyond the intended fixed height. Consider adding a way to suppress inline messages for table cells (and instead show an error border/tooltip), or avoid passing error down in this context.
| error={error} | |
| loading={loading} |
| "focus-within:outline-none focus-within:ring-1 focus-within:ring-offset-1", | ||
| "group focus-within:border-f1-border-hover focus-within:ring-1 focus-within:ring-f1-border-hover", | ||
| ], | ||
| transparent && "h-full w-full " |
There was a problem hiding this comment.
Minor: there’s trailing whitespace in the transparent && "h-full w-full " class string. Consider removing it to keep classnames clean (also makes twMerge output easier to reason about).
| transparent && "h-full w-full " | |
| transparent && "h-full w-full" |
|
Can you add a description pls? |
RafaMellado
force-pushed
the
editable-table/design
branch
from
5a71400 to
5a3ac8c
Compare
RafaMellado
force-pushed
the
editable-table/design
branch
from
5a3ac8c to
8dce903
Compare
| <div | ||
| className={cn( | ||
| editableColumn.align === "right" ? "justify-end" : "", | ||
| "flex" | ||
| "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" |
There was a problem hiding this comment.
Readonly cells conditionally omit the right border via !isLastColumn && ..., but editable cells render TextCell/BaseCell without any knowledge of isLastColumn, so the last editable column will still render a right border. Consider threading isLastColumn (or showRightBorder) into the editable cell component so borders are consistent between editable and readonly cells/headers.
| /** Indicates the source visualization type */ | ||
| fromVisualization?: "table" | "editableTable" | ||
| } & VisualizationOptions |
There was a problem hiding this comment.
fromVisualization is introduced on the generic CollectionProps, but it’s currently used as a TableCollection-only styling/behavior switch. This broadens the public props surface for every visualization and creates coupling to specific table variants. Consider moving this flag to TableCustomizationProps/table-specific options (or replacing it with a more focused table variant prop) so other visualizations don’t inherit an unrelated prop.
| /** Indicates the source visualization type */ | |
| fromVisualization?: "table" | "editableTable" | |
| } & VisualizationOptions | |
| } & VisualizationOptions | |
| /** | |
| * Table-specific visualization options. | |
| * | |
| * This type is intended to be used only by table visualizations so that | |
| * table-only styling/behavior flags do not leak into other visualizations. | |
| */ | |
| export type TableVisualizationOptions = { | |
| /** Indicates the source table visualization variant */ | |
| fromVisualization?: "table" | "editableTable" | |
| } |
| tableWithChildren, | ||
| }} | ||
| className={ | ||
| CellRenderer |
There was a problem hiding this comment.
The TableCell styling is changed for any cellRenderer usage (fixed height + p-0 + removing first:pl-6/last:pr-6). This is a behavioral change for TableCollection customization: wrappers that only want to augment default rendering will now lose standard table padding/height. Consider scoping this styling to the editable-table variant (e.g. gate it behind fromVisualization === "editableTable" or a dedicated flag) so existing cellRenderer consumers keep the default cell layout.
| CellRenderer | |
| CellRenderer && fromVisualization === "editableTable" |
There was a problem hiding this comment.
Not necessary for now
...taCollection/visualizations/collection/EditableTable/__tests__/EditableCellRenderer.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/react/src/experimental/OneDataCollection/visualizations/collection/EditableTable/components/EditableCellRenderer.tsx:102
isLastColumnis only used in the read-only branch to remove the right border, but editable cells always renderBaseCellwhich applies a right border by default. This will leave a right border on the last editable column while the last read-only column has none. Consider passingisLastColumn(orshowRightBorder) into the cell component and wiring it through toBaseCellso borders are consistent.
if (isEditable && hasId && cellEditType) {
const CellComponent = editableCellMap[cellEditType]
if (CellComponent) {
const value = getCellValue(localItem, editableColumn)
const error = editableColumn.id
? cellErrors[editableColumn.id]
: undefined
const loading = editableColumn.id
? (cellLoading[editableColumn.id] ?? false)
: false
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions -- stops cell navigation (href/onClick) from firing when interacting with the editor
<div
className="pointer-events-auto h-full"
onClickCapture={(e) => e.stopPropagation()}
onMouseDownCapture={(e) => e.stopPropagation()}
>
<CellComponent
label={editableColumn.label}
value={value}
align={editableColumn.align}
error={error}
loading={loading}
onChange={(v) => {
if (editableColumn.id) {
handleCellChange(editableColumn.id, v)
}
}}
/>
</div>
| @@ -0,0 +1,26 @@ | |||
| import { ReactNode } from "react" | |||
There was a problem hiding this comment.
ReactNode is a type-only import here. Use import type { ReactNode } from "react" to avoid emitting an unnecessary runtime import and to match the type-import pattern used elsewhere in the codebase.
| import { ReactNode } from "react" | |
| import type { ReactNode } from "react" |
| import { Input } from "@/experimental/Forms/Fields/Input" | ||
| import { cn } from "@/lib/utils" | ||
|
|
||
| import { EditableCellProps } from "." |
There was a problem hiding this comment.
EditableCellProps is a type-only import. Prefer import type { EditableCellProps } from "." to prevent generating a runtime import and to keep type/value imports consistent.
| import { EditableCellProps } from "." | |
| import type { EditableCellProps } from "." |
RafaMellado
force-pushed
the
editable-table/design
branch
from
e447126 to
2042afa
Compare
|
@RafaMellado I've opened a new pull request, #3584, to work on those changes. Once the pull request is ready, I'll request review from you. |
Grabacion.de.pantalla.2026-03-03.a.las.16.17.31.mov