Skip to content

Commit 9844bbf

Browse files
fix(components,plugin-list): inline edit marks a row modified only on a real change, draws one Actions column, and the Edit inline toggle reports aria-pressed (objectui#11816) (#11896)
Fixes #11816 Clause-②: no Grid inline edit, the three defects the card measured on showcase Tasks → *Edit inline*: ## 1. A row is modified only by a real change **Mechanism (measured).** `data-table`'s `saveEdit` and `stageEdit` staged every committed value into `pendingChanges`. A cell opened and left without a change still commits: a built-in editor commits on blur, and a host-injected editor (the `@object-ui/fields` widget `ObjectGrid` hands in) commits on the document pointerdown listener's click-outside. So "1 row modified · Cancel All · Save All (1)" appeared for a row nobody had changed. **Fix.** Both staging paths now go through one pure helper, `stageCellChange`. It stages the value only when `isUnchangedCellValue` says it differs from the value the row loaded with. Otherwise it removes the cell's entry, and the row's entry once no cell on it is staged. That last step is what makes staging the loaded value back un-mark the row. The equality rule covers only the round trips the editors make: - the emptiness floor, `isEmptyValue` from `@object-ui/core` (`null`, `undefined`, `''`, `[]`). `startEdit` seeds a `null` cell's editor with `''`; - a number against its decimal string. The built-in number editor stores the input's string; - a multi-value set in another order. `MultiSelectField`'s toggle appends; - two objects that serialize alike. Nothing else is equal. The failure direction is asymmetric: calling a change "unchanged" would drop an edit, as `isSameStoredValue` in `@object-ui/plugin-form` also says of its own rule. The ref-based staging is kept as it was: the `editingCellRef` re-entry guard, the `editRevertRef` snapshot, and `stageEdit`'s functional update. **PM hypothesis partly falsified: "a picklist code against its label".** No producer puts a label in the loaded row. The showcase seeds store codes (`priority: 'high'`). `SingleSelectField` and `MultiSelectField` hand back `option.value`. The select cell renders by resolving the code. The table also holds no options it could map a label through. Equating a label with a code would be a consumer-side tolerance, so it is not implemented. ## 2. One "Actions" column (the choice: a distinct header, not a merge) **Mechanism (measured).** With inline edit on, `ObjectGrid` draws its own row-menu column (`_actions`, headed by `grid.actions`). It also passes `rowActions: true` to `data-table`, whose trailing column, headed by `common.actions`, only ever holds a modified row's cancel/save pair there. The result is two "Actions" headers. The pin below counts 2 on the base. The second header was a bare string in the cell's larger default type, while every data-column label sits in a `text-xs` muted span. **Choice: a distinct, consistently styled header.** When the trailing column can only hold that pair, `data-table` heads it with a pencil glyph. The column can only hold the pair when the table is editable, has `onRowSave` or `onBatchSave`, and is given none of the menu inputs `DataTableRowActionsMenu` reads (`onRowEdit`, `onRowDelete`, a non-empty `rowActionDefs` with `onRowActionDef`). The pencil is the one the *Edit inline* toggle carries, with the accessible name and tooltip `table.edit` ("Edit", an existing key in every pack). A column that can hold a row menu keeps "Actions", now in the data-column header type. Why not merge: - One column would need `ObjectGrid`'s `_actions` cell to render the table's per-row save/cancel, or the table to render `ObjectGrid`'s `RowActionMenu`. Either needs a new seam: a third `TableColumn.cell` argument, an exported context, or a column-placement key. That is a published-contract change, which the claim's stop conditions route to a re-claim with `Clause-②: yes`. - A text header would need a new `packages/i18n` key, the other stop condition. - Dropping the per-row save/cancel pair would remove an existing affordance without a ruling. ## 3. The *Edit inline* toggle reports its state The toggle is `ListView`'s toolbar button (`data-testid="toolbar-inline-edit-toggle"`) in `@object-ui/plugin-list`, not in `ObjectGrid`. It now sets `aria-pressed` from the inline-edit state. The compact-toolbar entry in `ViewSettingsPopover` is a native checkbox and already exposes its state. ## Files, and the claim's surface - In the claim: `packages/components/src/renderers/complex/data-table.tsx`, its test `packages/components/src/__tests__/data-table-noop-edit-11816.test.tsx`, `.changeset/11816-inline-edit-noop.md`. - **Outside the claim's list, landed on the producer side as the dispatch directs:** `packages/plugin-list/src/ListView.tsx` (one attribute) and one new case in `packages/plugin-list/src/__tests__/ListView.test.tsx`. The toggle lives there; `ObjectGrid.tsx` renders no toggle. Also new: `packages/plugin-grid/src/__tests__/inlineEditNoopAndHeader-11816.test.tsx`, a test only. It drives the real `ObjectGrid` with the real `@object-ui/fields` select, the card's own shape. - `packages/plugin-grid/src/ObjectGrid.tsx` is not edited, and nothing in the claim's "Not on it" list is touched. - No export, prop, type member or `packages/i18n` key is added: `stageCellChange`, `isUnchangedCellValue` and `isSameCellScalar` are module-private. - Patch changeset for `@object-ui/components` and `@object-ui/plugin-list`. `@object-ui/plugin-grid` gets no entry, because only a test file changed there. ## Gates (final head `4bdd909`, except where a row names `bdbf14a`; `bdbf14a`..`4bdd909` restores one dropped space in `data-table.tsx`) - `pnpm exec vitest run packages/components/src/__tests__/data-table-noop-edit-11816.test.tsx packages/plugin-grid/src/__tests__/inlineEditNoopAndHeader-11816.test.tsx packages/plugin-list/src/__tests__/ListView.test.tsx` at `4bdd909`: `Test Files 3 passed (3)` · `Tests 160 passed (160)`. - `pnpm --filter @object-ui/components type-check` at `4bdd909`: exit 0, with the script name echoed. `pnpm --filter @object-ui/plugin-list type-check` and `pnpm --filter @object-ui/plugin-grid type-check` at `bdbf14a`: exit 0. Each package's `tsconfig.test.json` program lists the touched test file (`tsc --listFilesOnly`, one hit each). - Dependency closure, built first at `bdbf14a`: `pnpm --workspace-concurrency=2 --filter '@object-ui/components^...' --filter '@object-ui/plugin-list^...' --filter '@object-ui/plugin-grid^...' build`, exit 0 (`Scope: 14 of 47 workspace projects`). - The three packages' full suites at `bdbf14a`: `pnpm exec vitest run --maxWorkers=2 packages/components/ packages/plugin-list/ packages/plugin-grid/`, giving `Test Files 671 passed | 1 skipped (672)` · `Tests 6740 passed | 24 skipped (6764)`. - `node scripts/check-changeset-presence.mjs`: `✅ 5 source file(s) of 3 released package(s) changed, and this change declares 1 changeset(s): .changeset/11816-inline-edit-noop.md.` - `pnpm check:new-line-citations`: `VERDICT new-cross-file-line-citations: 0 new citation(s), enforcement report-only -> exit 0`. - `pnpm check:control-bytes`: `✅ check-control-bytes: OK`. - `pnpm check:i18n-keys`: exit 0. `table.edit` resolves against the en pack. - `pnpm check:component-surface-parity`: `REPORT-ONLY -- exit 0`. No `data-table` input or registration changed. - `pnpm exec eslint --format json` on the 5 changed `.ts`/`.tsx` files: 5 files, 0 errors. This narrowing is a measurement, for three reasons: - its population is the config's `**/*.{ts,tsx}` blocks applied to the changed files; - `eslint.config.js` sets no `parserOptions.project` or `projectService`, so linting is per-file and this diff cannot move a verdict on an untouched file; - the three modified files carry the same warning counts as at `cef0eee` (34, 188, 26). The two new test files carry `no-explicit-any` warnings, as their siblings do. **Reverse verification** (one-shot, committed first). `data-table.tsx` and `ListView.tsx` were written back to `cef0eee`. The on-disk markers read `stageCellChange` 0, `rowActionsColumnIsEditOnly` 0 and `aria-pressed={inlineEdit}` 0. The same three test files then ran: `Tests 12 failed | 148 passed (160)`. Every new pin goes red, except the control "a column that can hold a row menu stays Actions", which is green on both trees. The `ObjectGrid` pin reads `expected [ th, th ] to have a length of 1 but got 2`. The files were restored with `git checkout HEAD --`: both blob hashes match `HEAD` and `git diff HEAD` is empty. Tests resolve `@object-ui/*` to `src` (`vitest.config.mts` aliases), so no build sat between the edit and the run. NOT MEASURED: a real-browser pass on the console against a showcase backend; the repo-wide `pnpm lint` and `pnpm test` (CI). ## Acceptance notes (observed, not filed) - `onCellChange` still fires on every commit, a no-op commit included. Only the staging is gated, because gating the callback the same way could leave a host that tracked an earlier staged value stale. Carrier: none. - Two equality rules now exist for "did this value change": `isSameStoredValue` (plugin-form: `''` is not blank, numbers never equal strings) and `isUnchangedCellValue` here. Each matches its own round trip; unifying them is a design question. Carrier: none. - Read from source, not reproduced: `SingleSelectField`'s cascade clear compares the stored value with option codes exactly, while the select cell resolves a case-variant code. A stored `Referral` against a declared `referral` would be cleared to `null` when the editor mounts, and that is staged, now as a real change. Carrier: none. - The per-row cancel/save buttons carry English `title` strings ("Cancel changes", "Save row") outside the language packs, unchanged here. Carrier: none. Session: `https://claude.ai/code/session_01MgfduSkFrfM3eorB3UGfAU` --- _Generated by [Claude Code](https://claude.ai/code/session_01MgfduSkFrfM3eorB3UGfAU)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a476dee commit 9844bbf

7 files changed

Lines changed: 511 additions & 20 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@object-ui/components': patch
3+
'@object-ui/plugin-list': patch
4+
---
5+
6+
Grid inline edit marks a row modified only when a value really changed, draws one "Actions" column, and the "Edit inline" toggle reports its state (objectui#11816).
7+
8+
- **A row is modified only by a real change.** The data table staged every committed cell edit, so clicking into a cell and out again showed "1 row modified · Cancel All · Save All (1)" for a row nobody had changed. An edit is now staged only when the value differs from the one the row loaded with. `null`, `undefined`, `''` and `[]` count as the same empty value, a number equals the decimal string that spells it (`5` and `'5'`), and a multi-value set equals the same set in another order. Staging the loaded value back removes the cell's edit, and the row is no longer counted as modified once none of its cells are.
9+
- **One "Actions" column.** With inline edit on, the trailing column that only holds a modified row's cancel and save buttons was headed "Actions" too, beside the grid's own row-menu column, and in larger type. When that column can only hold those buttons (the table is editable, has a save handler, and is given no row-menu handler), its header is now a pencil icon with the accessible name "Edit" (the existing `table.edit` key). A column that can hold a row menu keeps the "Actions" header, now in the same small muted type as the other column headers.
10+
- **The "Edit inline" toolbar toggle sets `aria-pressed`** to whether inline edit is on.
11+
12+
No export, prop, type member or language-pack key is added.

‎content/docs/plugins/plugin-grid.mdx‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -776,13 +776,16 @@ const persistence = (
776776
```
777777

778778
**Features:**
779-
- **Pending changes tracking**: edit multiple cells across rows before saving
779+
- **Pending changes tracking**: edit multiple cells across rows before saving; a
780+
cell edited back to the value it loaded with is not a pending change, and its
781+
row is no longer counted as modified
780782
- **Visual indicators**: modified rows highlighted in amber, modified cells in bold
781783
- **Row-level save/cancel**: individual row save and cancel buttons
782784
- **Batch operations**: Save All and Cancel All buttons for bulk actions
783785
- **Flexible callbacks** — all three are `ObjectGridComponentProps`, never schema
784786
keys: `onRowSave` for a single row, `onBatchSave` for many, `onCellChange` for
785-
each staged cell edit
787+
each committed cell edit, including one that leaves the value as loaded (only a
788+
real change is staged)
786789

787790
## TypeScript Support
788791

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectui#11816 — inline edit on an `editable` data-table:
11+
*
12+
* 1. a row counts as modified only when a value differs from the one it
13+
* loaded with. Every commit used to be staged, so opening a cell and
14+
* leaving it as it was showed "1 row modified · Save All (1)";
15+
* 2. staging the loaded value back un-marks the row;
16+
* 3. the trailing column that only ever holds a modified row's cancel/save
17+
* pair is not headed "Actions" — `ObjectGrid` already draws a host column
18+
* of that name beside it — and the "Actions" label takes the data-column
19+
* header styling instead of the cell's larger default type.
20+
*
21+
* Each case drives the real renderer through the user's own gestures and reads
22+
* the toolbar the user reads.
23+
*/
24+
import { describe, it, expect, vi } from 'vitest';
25+
import { fireEvent, screen } from '@testing-library/react';
26+
import '@testing-library/jest-dom';
27+
import React from 'react';
28+
import { renderComponent } from './test-utils';
29+
// Registers the renderers at module scope, NOT inside a `beforeAll`
30+
// (object-ui/no-dynamic-import-in-test-hook, objectui#3010/#3021).
31+
import '../renderers';
32+
33+
/** A host-injected editor pair: a multi-value picker and a discrete select. */
34+
const renderCellEditor = ({ column, stage, commit }: any) => {
35+
if (column.accessorKey === 'tags') {
36+
return (
37+
<div>
38+
<button data-testid="tags-reorder" onClick={() => stage(['b', 'a'])}>reorder</button>
39+
<button data-testid="tags-drop" onClick={() => stage(['a'])}>drop</button>
40+
<button data-testid="tags-restore" onClick={() => stage(['a', 'b'])}>restore</button>
41+
</div>
42+
);
43+
}
44+
if (column.accessorKey === 'priority') {
45+
// A real select's option sits in a portal; this stand-in sits in the cell,
46+
// so keep its click from re-entering the cell's own click-to-edit.
47+
return (
48+
<button
49+
data-testid="pick-high"
50+
onClick={(e) => {
51+
e.stopPropagation();
52+
commit('high');
53+
}}
54+
>
55+
High
56+
</button>
57+
);
58+
}
59+
return null;
60+
};
61+
62+
const makeSchema = (extra: Record<string, unknown> = {}) =>
63+
({
64+
type: 'data-table' as const,
65+
editable: true,
66+
singleClickEdit: true,
67+
rowActions: true,
68+
onBatchSave: vi.fn(),
69+
renderCellEditor,
70+
columns: [
71+
{ header: 'Name', accessorKey: 'name' },
72+
{ header: 'Qty', accessorKey: 'qty', type: 'number' },
73+
{ header: 'Note', accessorKey: 'note' },
74+
{ header: 'Tags', accessorKey: 'tags' },
75+
{ header: 'Priority', accessorKey: 'priority' },
76+
],
77+
data: [{ id: '1', name: 'Alpha', qty: 5, note: null, tags: ['a', 'b'], priority: 'high' }],
78+
...extra,
79+
}) as any;
80+
81+
/** The `<td>` of the first row in column `index`, per `makeSchema`'s order. */
82+
const cell = (container: HTMLElement, index: number) =>
83+
container.querySelectorAll('tbody tr')[0].querySelectorAll('td')[index] as HTMLElement;
84+
const NAME = 0;
85+
const QTY = 1;
86+
const NOTE = 2;
87+
const TAGS = 3;
88+
const PRIORITY = 4;
89+
90+
/** What the toolbar says about staged edits; `null` when nothing is staged. */
91+
const modified = () => screen.queryByText(/rows? modified/)?.textContent ?? null;
92+
93+
/** Open `td`'s built-in editor, run `edit` on its input, then blur it. */
94+
function editBuiltIn(td: HTMLElement, edit: (input: HTMLInputElement) => void = () => {}) {
95+
fireEvent.click(td);
96+
const input = td.querySelector('input') as HTMLInputElement;
97+
expect(input).toBeTruthy();
98+
edit(input);
99+
fireEvent.blur(input);
100+
}
101+
102+
describe('data-table inline edit — a row is modified only by a real change (objectui#11816)', () => {
103+
it('a text cell opened and left as it was stages nothing', () => {
104+
const { container } = renderComponent(makeSchema());
105+
editBuiltIn(cell(container, NAME));
106+
expect(modified()).toBeNull();
107+
expect(screen.queryByText(/Save All/)).toBeNull();
108+
});
109+
110+
it('an empty cell left empty stages nothing (the editor seeds it with an empty string)', () => {
111+
const { container } = renderComponent(makeSchema());
112+
editBuiltIn(cell(container, NOTE), (input) => expect(input.value).toBe(''));
113+
expect(modified()).toBeNull();
114+
});
115+
116+
it('a number retyped as the same numeral stages nothing; a different number does', () => {
117+
const { container } = renderComponent(makeSchema());
118+
// Through `7` first so the editor really holds the input's STRING `'5'`.
119+
editBuiltIn(cell(container, QTY), (input) => {
120+
fireEvent.change(input, { target: { value: '7' } });
121+
fireEvent.change(input, { target: { value: '5' } });
122+
});
123+
expect(modified()).toBeNull();
124+
125+
editBuiltIn(cell(container, QTY), (input) => fireEvent.change(input, { target: { value: '6' } }));
126+
expect(modified()).toBe('1 row modified');
127+
});
128+
129+
it('staging the loaded value back un-marks the row', () => {
130+
const { container } = renderComponent(makeSchema());
131+
editBuiltIn(cell(container, QTY), (input) => fireEvent.change(input, { target: { value: '6' } }));
132+
expect(modified()).toBe('1 row modified');
133+
134+
editBuiltIn(cell(container, QTY), (input) => fireEvent.change(input, { target: { value: '5' } }));
135+
expect(modified()).toBeNull();
136+
expect(screen.queryByText(/Save All/)).toBeNull();
137+
});
138+
139+
it('a discrete editor committing the loaded code stages nothing', () => {
140+
const { container } = renderComponent(makeSchema());
141+
fireEvent.click(cell(container, PRIORITY));
142+
fireEvent.click(screen.getByTestId('pick-high'));
143+
expect(screen.queryByTestId('pick-high')).toBeNull(); // committed and closed
144+
expect(modified()).toBeNull();
145+
});
146+
147+
it('an injected editor left by a click outside stages nothing (the reported gesture)', () => {
148+
const { container } = renderComponent(makeSchema());
149+
fireEvent.click(cell(container, PRIORITY));
150+
expect(screen.getByTestId('pick-high')).toBeInTheDocument();
151+
fireEvent.pointerDown(document.body);
152+
expect(screen.queryByTestId('pick-high')).toBeNull(); // left edit mode
153+
expect(modified()).toBeNull();
154+
});
155+
156+
it('a multi-value set in another order is the same value; a smaller set is not', () => {
157+
const { container } = renderComponent(makeSchema());
158+
fireEvent.click(cell(container, TAGS));
159+
fireEvent.click(screen.getByTestId('tags-reorder'));
160+
expect(modified()).toBeNull();
161+
fireEvent.click(screen.getByTestId('tags-drop'));
162+
expect(modified()).toBe('1 row modified');
163+
fireEvent.click(screen.getByTestId('tags-restore'));
164+
expect(modified()).toBeNull();
165+
});
166+
});
167+
168+
describe('data-table inline edit — the edit column is not a second "Actions" (objectui#11816)', () => {
169+
it('the save-only column is headed by its own name, not "Actions"', () => {
170+
renderComponent(makeSchema());
171+
expect(screen.queryByRole('columnheader', { name: 'Actions' })).toBeNull();
172+
expect(screen.getByRole('columnheader', { name: 'Edit' })).toBeInTheDocument();
173+
});
174+
175+
it('a column that can hold a row menu stays "Actions"', () => {
176+
renderComponent(makeSchema({ onRowEdit: () => {} }));
177+
expect(screen.getByRole('columnheader', { name: 'Actions' })).toBeInTheDocument();
178+
expect(screen.queryByRole('columnheader', { name: 'Edit' })).toBeNull();
179+
});
180+
181+
it('the "Actions" label is set in the data-column header type', () => {
182+
renderComponent(makeSchema({ editable: false, onRowEdit: () => {} }));
183+
const label = (name: string) =>
184+
screen.getByRole('columnheader', { name }).querySelector('span') as HTMLElement;
185+
for (const token of ['text-xs', 'font-medium', 'text-muted-foreground']) {
186+
expect(label('Name')).toHaveClass(token);
187+
expect(label('Actions')).toHaveClass(token);
188+
}
189+
});
190+
});

0 commit comments

Comments
 (0)