Add inline cell editing to object-grid with batch save support#390
Add inline cell editing to object-grid with batch save support#390
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 参考以下提示词的功能点进行修改,具体的细节结合现在的代码你来定。 这是一个非常清晰的开发阶段:从“只读视图”升级为“可编辑视图”。
第二步:条件渲染与组件桥接 (Conditional Rendering)
第三步:数据保存与全流程闭环 (Save & Feedback)
第四步:Airtable 风格的细节优化 (Airtable Polish)
总结开发流程
|
The comment appears to be cut off. Could you please provide the complete prompt/requirements? I can see it starts with "提示词 1:我目前的 DataGrid 组件已经可以遍历数据并调用" but the rest is missing. What specific changes would you like me to make to the current implementation? |
|
@copilot 简单地说,在快捷编辑的模式下,需要考虑多个字段的编辑以及数据保存,包括单行的保存和多行的批量保存。 |
There was a problem hiding this comment.
Pull request overview
This PR implements inline cell editing for object-grid and data-table components, enabling spreadsheet-like editing with keyboard navigation. It adds global and per-column editable controls with callbacks for handling cell value changes.
Changes:
- Added
editableboolean flag andonCellChangecallback to type schemas for both global and column-level control - Implemented inline editing in data-table renderer with Enter/Escape keyboard shortcuts and double-click activation
- Integrated editing support into object-grid component by passing through the new props
- Added comprehensive documentation and Storybook examples demonstrating the editing features
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/data-display.ts | Added editable property to TableColumn and DataTableSchema, plus onCellChange and onRowClick callbacks |
| packages/components/src/renderers/complex/data-table.tsx | Implemented cell editing state management, keyboard handlers, and edit mode rendering with Input component |
| packages/plugin-grid/src/ObjectGrid.tsx | Updated to pass editable and onCellChange props through to underlying data-table |
| packages/plugin-grid/README.md | Added "Inline Editing" section with usage examples and feature documentation |
| content/docs/plugins/plugin-grid.mdx | Added documentation for inline editing feature with JSON examples |
| packages/components/src/stories-json/object-grid.stories.tsx | Added EditableGrid story demonstrating inline editing capabilities |
| packages/components/src/stories-json/data-table.stories.tsx | Added EditableTable story with example data and onCellChange handler |
| packages/components/src/renderers/complex/tests/data-table-editing.test.tsx | Added comprehensive test suite covering edit mode activation, saving, canceling, and column-level control |
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for activating edit mode by pressing Enter on a focused cell. The implementation supports this feature (line 388-392 in data-table.tsx), but there's no test to verify it works correctly. Consider adding a test that focuses a cell and presses Enter to ensure edit mode is activated.
| ref={editInputRef} | ||
| value={editValue} | ||
| onChange={(e) => setEditValue(e.target.value)} | ||
| onKeyDown={handleEditKeyDown} |
There was a problem hiding this comment.
When a cell is in edit mode, clicking on the input field or the cell itself will trigger the onRowClick handler (if defined). This is likely unintended behavior - clicking within an editing cell should not trigger row click events. Consider adding logic to check if a cell is currently being edited (similar to how the code checks for buttons and checkboxes on lines 570-571) or adding e.stopPropagation() to the input's onClick handler.
| onKeyDown={handleEditKeyDown} | |
| onKeyDown={handleEditKeyDown} | |
| onClick={(e) => e.stopPropagation()} |
| // Don't save if we're in cancelled state (unless forced) | ||
| if (!force && editingCell === null) return; | ||
|
|
There was a problem hiding this comment.
This condition is unreachable and redundant. Line 359 already returns early if editingCell is null, so line 362 checking editingCell === null will never evaluate to true. This appears to be dead code that should be removed.
| // Don't save if we're in cancelled state (unless forced) | |
| if (!force && editingCell === null) return; | |
| }} | ||
| onDoubleClick={() => isEditable && startEdit(rowIndex, col.accessorKey, cellValue)} | ||
| onKeyDown={(e) => handleCellKeyDown(e, rowIndex, col.accessorKey)} | ||
| tabIndex={0} |
There was a problem hiding this comment.
All table cells receive tabIndex={0} regardless of whether they are editable or not. This creates unnecessary tab stops and degrades keyboard navigation accessibility. The tabIndex should only be set conditionally when a cell is actually editable (when isEditable is true). Non-editable cells, including the ID column with editable: false, should not be in the tab order.
| tabIndex={0} | |
| tabIndex={isEditable ? 0 : undefined} |
| ref={editInputRef} | ||
| value={editValue} | ||
| onChange={(e) => setEditValue(e.target.value)} | ||
| onKeyDown={handleEditKeyDown} |
There was a problem hiding this comment.
The edit input is missing an onBlur handler to exit edit mode when the user clicks or tabs away from the cell. Currently, users can only save (Enter) or cancel (Escape) the edit. Without an onBlur handler, clicking outside the input cell leaves it in edit mode, which is inconsistent with standard spreadsheet behavior. Consider adding an onBlur handler that either auto-saves or cancels the edit based on the desired UX (the PR description mentions "No auto-save on blur" but there's no blur handler at all currently).
| onKeyDown={handleEditKeyDown} | |
| onKeyDown={handleEditKeyDown} | |
| onBlur={() => { | |
| // Exit edit mode on blur without auto-saving | |
| setEditingCell(null); | |
| setEditValue(''); | |
| }} |
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
There was a problem hiding this comment.
Unused variable container.
| const { container } = render(<DataTableRenderer schema={schema} />); | |
| render(<DataTableRenderer schema={schema} />); |
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
There was a problem hiding this comment.
Unused variable container.
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
There was a problem hiding this comment.
Unused variable container.
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
There was a problem hiding this comment.
Unused variable container.
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
| if (!editingCell) return; | ||
|
|
||
| // Don't save if we're in cancelled state (unless forced) | ||
| if (!force && editingCell === null) return; |
Check warning
Code scanning / CodeQL
Comparison between inconvertible types Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
In general, when a variable is already guarded against being null or undefined, performing an additional === null comparison inside the guarded block is both redundant and misleading. It can also trigger static analysis warnings about comparing inconvertible or impossible types.
The best fix here is to remove the logically impossible check if (!force && editingCell === null) return; from saveEdit. The function already exits early if editingCell is falsy (null or undefined), so any code that follows can safely assume editingCell is non-null. Removing this condition does not alter existing, reachable functionality and cleans up the control flow.
Concretely:
- In
packages/components/src/renderers/complex/data-table.tsx, inside thesaveEditfunction:-
Delete the block:
// Don't save if we're in cancelled state (unless forced) if (!force && editingCell === null) return;
-
No additional imports, methods, or definitions are needed.
| @@ -368,9 +368,6 @@ | ||
| const saveEdit = (force: boolean = false) => { | ||
| if (!editingCell) return; | ||
|
|
||
| // Don't save if we're in cancelled state (unless forced) | ||
| if (!force && editingCell === null) return; | ||
|
|
||
| const { rowIndex, columnKey } = editingCell; | ||
| const globalIndex = (currentPage - 1) * pageSize + rowIndex; | ||
| const row = sortedData[globalIndex]; |
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
In general, unused variables in tests should be removed or replaced with a destructuring that omits them, to improve clarity and satisfy static analysis tools. Here, in the second it('should call onCellChange when cell value is edited', ...) block, we destructure { container } from the render call but never use it.
The best fix without changing existing functionality is to call render without capturing container in this second test. We can simply remove the destructuring and call render(<DataTableRenderer schema={schema} />);. This change is local to the second test, at line 71 in packages/components/src/renderers/complex/__tests__/data-table-editing.test.tsx. No new imports or helper methods are needed.
| @@ -68,7 +68,7 @@ | ||
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); | ||
| render(<DataTableRenderer schema={schema} />); | ||
|
|
||
| // Find the first editable cell (name column) | ||
| const nameCell = screen.getByText('John Doe').closest('td'); |
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
In general, to fix unused variable issues, either remove the variable (and its binding) or start using it meaningfully. Here, the best fix that does not change existing behavior is to stop destructuring container from the render result in the second test, since the test does not use it.
Concretely, in packages/components/src/renderers/complex/__tests__/data-table-editing.test.tsx, locate the line within the "should not allow editing when editable is false" test:
const { container } = render(<DataTableRenderer schema={schema} />);Replace it with a call to render that does not bind the result to container, e.g.:
render(<DataTableRenderer schema={schema} />);This preserves the render side effect while eliminating the unused variable. No new imports, methods, or definitions are required.
| @@ -118,7 +118,7 @@ | ||
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); | ||
| render(<DataTableRenderer schema={schema} />); | ||
|
|
||
| // Find a cell | ||
| const nameCell = screen.getByText('John Doe').closest('td'); |
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
In general, to fix an unused variable reported by static analysis, either remove the variable (and any destructuring pattern that binds it) or start using it meaningfully. Here, the test does not need container at all: all queries are done via screen and DOM traversal from specific cells. The simplest and safest fix is to stop destructuring container from the render call and instead just call render without capturing its return value.
Concretely, in packages/components/src/renderers/complex/__tests__/data-table-editing.test.tsx, within the it('should respect column-level editable flag', ...) block, replace const { container } = render(<DataTableRenderer schema={schema} />); with a plain render(<DataTableRenderer schema={schema} />);. No imports or additional definitions are required, and no behavior changes, since container was never read.
| @@ -150,7 +150,7 @@ | ||
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); | ||
| render(<DataTableRenderer schema={schema} />); | ||
|
|
||
| // Try to edit ID column (which has editable: false) | ||
| const idCell = screen.getByText('1').closest('td'); |
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
To fix the problem, remove the unused container variable from the test so that CodeQL no longer reports it and the code is cleaner. The simplest and safest change is to call render without assigning its return value in this test, since none of the returned helpers are used.
Concretely, in packages/components/src/renderers/complex/__tests__/data-table-editing.test.tsx, in the "should cancel edit on Escape key" test, replace:
const { container } = render(<DataTableRenderer schema={schema} />);with:
render(<DataTableRenderer schema={schema} />);No additional imports or definitions are needed, and this does not alter existing functionality because container was not used.
| @@ -181,7 +181,7 @@ | ||
| const DataTableRenderer = ComponentRegistry.get('data-table'); | ||
| if (!DataTableRenderer) throw new Error('DataTableRenderer not found'); | ||
|
|
||
| const { container } = render(<DataTableRenderer schema={schema} />); | ||
| render(<DataTableRenderer schema={schema} />); | ||
|
|
||
| // Find the first editable cell | ||
| const nameCell = screen.getByText('John Doe').closest('td'); |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
已更新 Storybook 示例 (commit 9ac7575)! data-table.stories.tsx:
object-grid.stories.tsx:
所有示例都包含详细的 JSDoc 说明和实际的保存回调演示。 |
📦 Bundle Size Report
Size Limits
|
|
@copilot storybook 中没有看到保存按钮和功能 |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
保存按钮是动态显示的,需要先编辑单元格才会出现 (commit 5830eff)。 如何查看保存按钮:
已在所有批量编辑示例的 JSDoc 中添加了详细的步骤说明:
这是正确的行为设计 - 保存按钮只在有待保存的更改时才显示,避免界面混乱。 |
|
Error: useSchemaContext must be used within a SchemaRendererProvider |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
已修复 (commit 33c9eab)! 问题是 data-table.stories.tsx 缺少 import { SchemaRenderer, SchemaRendererProvider } from '@object-ui/react';
import { createStorybookDataSource } from '@storybook-config/datasource';
const dataSource = createStorybookDataSource();
const renderStory = (args: any) => (
<SchemaRendererProvider dataSource={dataSource}>
<SchemaRenderer schema={args as unknown as BaseSchema} />
</SchemaRendererProvider>
);所有故事现在都使用 |
Implementation Plan: Object-Grid Row Editing Feature
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.