Repository navigation
Let custom buttons and collection components commit with handleEdit (#418) - #421
Conversation
A custom button's `onClick` receives a third argument, `{ handleEdit }`,
and its `Element` receives `handleEdit` and `canEdit` as props.
`handleEdit(value)` commits a new value at the button's own node through
the same pipeline as an edit: `onUpdate`, the `onEditEvent` stream, and
the node's inline error and `onError` on rejection. It isn't gated by
`allowEdit`; `canEdit` lets a button hide itself on read-only nodes.
Each node supplies the function, so a rejection settles on that node: a
value row reuses its `handleEdit(value)`, and a collection row submits
and reports through `onError`. `onClick` is optional in the type, as the
README and implementation already treated it. `CustomButtonElementProps`
is exported.
The custom-buttons demo example gains a "Toggle read" button and an
`onUpdate` toast showing which changes pass through it.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A collection's `handleEdit` takes an optional value, as a value node's does: `handleEdit(value)` commits that value as-is, and `handleEdit()` commits the raw-JSON edit buffer. So `handleEdit(value)` is the commit call for value components, collection components and custom buttons alike. A collection component's `setValue` keeps committing, but runs through `handleEdit(value)`, so a rejection now shows the node's inline error and reaches `onError`, where before it fired only `updateError`. Making it buffer-only on collections, to match value components, is a breaking change left as a TO-DO for v3. The playlist demo example commits with `handleEdit`, and the README documents both props. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bundle size impact
|
| Format | Base raw | PR raw | Δ raw | Base gzip | PR gzip | Δ gzip |
|---|---|---|---|---|---|---|
| esm | 59.00 KB | 59.33 KB | 🔺 +338 B (+0.56%) | 20.92 KB | 20.99 KB | 🔺 +74 B (+0.35%) |
| cjs | 60.43 KB | 60.77 KB | 🔺 +347 B (+0.56%) | 20.94 KB | 21.01 KB | 🔺 +76 B (+0.35%) |
Measured from build/index.{cjs,esm}.js. Gzip at level 9.
CarlosNZ
left a comment
There was a problem hiding this comment.
This PR is in good shape. The core change is small and reuses the existing commit paths rather than adding a new one, and making a value node and a collection node both take handleEdit(value) is the right move for API consistency. On the PR head, the full suite passes (788), lint is clean, and compile's ts-prune output differs from main only by line numbers. I also checked a few edge cases with throwaway tests; the results are below.
Should fix before merge
1. Hiding a button by returning null doesn't stop its onClick, and the demo's toggle can damage data.
When a button has a label, its wrapper <button aria-label> renders whether or not Element returns null (ButtonPanels.tsx#L250). The empty button has no width, so a mouse can't hit it, but it's still in the accessibility tree and a screen reader can press it. Until now a stray press was mostly harmless. With handleEdit it becomes a write path, and the PR's own docs present "return null when !canEdit" as the way to protect a node.
Tested: on data { title: 'abc', locked: 'zz' }, with locked read-only, I pressed the hidden "Toggle read" button on both rows, using the demo's onClick:
{"title":{"0":"a","1":"b","2":"c","read":true},"locked":{"0":"z","1":"z","read":true}}
The strings were spread into objects, and that included the read-only row. The README's uppercaseButton would throw a TypeError on a number node in the same way.
The smallest fix is to repeat the check inside onClick, in both the README example and the demo. That only works cleanly if onClick also gets canEdit (next point).
2. onClick gets { handleEdit } but not canEdit.
Element gets both. Because of point 1, onClick is where the guard has to live, and right now it can't check permissions. Suggest passing { handleEdit, canEdit } as the third argument too, so both entry points get the same data.
Worth considering
3. Version number. The PR adds features and a new public type export (CustomButtonElementProps), and changes how a rejected collection setValue is reported. That makes it a minor release: 2.1.0, as the PR body suggests, rather than 2.0.4.
4. A button that turns a value into a collection leaves it collapsed. With collapse={1}, a button that turns 'abc' into { inner: 'abc' } shows the new object collapsed. The built-in type switch expands the new collection on purpose (#217). Either one setCollapseState(...) call in the explicit-value branch, or a sentence in the docs.
5. Merge and deploy order. The demo pins json-edit-react@2.0.3, so in npm mode the custom-buttons example throws when it destructures handleEdit from undefined. The PR body notes this, but it also means the demo mustn't be released until core is published and the dependency bumped.
Simplicity and consistency nits
- Two names for one wrapper:
handleEditFromButton(ValueNodeWrapper.tsx#L378) andcommitValue(CollectionNode.tsx#L331) are the same one-argument wrapper.commitValuein both files would be clearer. - Two types for
handleEdit:CustomComponentProps.handleEditis(value?: unknown) => void, while the button's version is(value: JsonData) => void. Changing the first to(value?: JsonData)would make them match without breaking anything in practice. - CHANGELOG length: the second bullet packs in a feature and a behaviour change; it could be cut down to a sentence plus the README link.
Checked and fine
- The collection
handleEdit's first parameter changing fromonCommittoinputValue. Every internal caller is updated:commitOppasses(undefined, onCommit),handleKeyPresscallsaction()with no arguments, and click or native events are skipped byisJsEvent. None of the@json-edit-react/componentscomponents call a collection'shandleEditwith an argument. - The type changes don't break existing code. A consumer's
Elementtyped asFC<{ nodeData }>still fits the wider props type, and makingonClickoptional only affects code that callsdef.onClickdirectly. - No performance cost.
EditButtonsisn't memoised, and the new handlers are recreated on each render, so they always read current state, asPERF-ARCHITECTURErequires. - The playlist flow (
handleEdit(next)thensetIsEditing(true)) works with noonUpdate, a sync one, an async one, and withhold(). Clearing the buffer when the commit lands is safe there.
An existing engine bug this PR makes easier to hit (not a blocker)
If node A is mid-edit and something commits at node B (Delete, a collection component's setValue, or now any custom button), submit() in EditingProvider.tsx wipes A's cancelOp / commitOp without closing A's session (and hold() overwrites A's session outright). Reproduced: when the user then opens node C, A's typed edit is neither committed nor reverted, and A keeps showing "TYPED" while data still holds "aaa". Delete does the same on main, so this PR didn't cause it, but custom buttons are a new easy way to trigger it. Best handled as a separate fix.
🤖 Generated with Claude Code
A labelled custom button rendered its wrapper <button aria-label> even when `Element` returned `null`. The empty button was zero-width but still in the accessibility tree, so assistive tech could press it, which with `handleEdit` is a write path. Empty wrappers also took a flex gap each. Both wrappers now carry `jer-custom-button`, and the stylesheet hides it while empty, so returning `null` removes the button entirely. The tests load the real stylesheet, since Jest stubs it. `onClick`'s third argument is renamed `context` and carries `canEdit` alongside `handleEdit`, so `onClick` and `Element` receive the same data. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…review) A custom button or component that commits an object through `handleEdit(value)` turns a value node into a collection, which a level-based `collapse` then showed collapsed. It now launches expanded, as the built-in type switch does (#217). The collapse broadcast is sent at the commit moment, so a rejected edit leaves nothing pending to open a collection that reaches that path later. Nits: the value node's wrapper is named `commitValue`, as in CollectionNode; `CustomComponentProps.handleEdit` is typed with `JsonData` (identical to `unknown`, but it reads like the button's); the collection CHANGELOG bullet is cut to a sentence and a link. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for the review. Responses below; the changes are in 1. Hidden buttons can still be pressed. Fixed at the root, not in the consumer's 2. 3 and 5. Not addressed in this PR. 4. A value turned into a collection stays collapsed. Fixed. When an explicit Nits. All three done. The wrapper is Existing engine bug. Reproduced, and filed as #422 with the two-part cause: callbacks cleared by an unrelated commit, and 🤖 Generated with Claude Code |
Closes #418.
Summary
A custom button had no way to commit a change through the edit pipeline: its
onClickreceived onlynodeDataand the event, so it had to call the host'ssetData, which skipsonUpdateandonEditEvent. This PR gives buttons ahandleEdit, and makeshandleEdit(value)the commit call for custom collection components too.1. Custom buttons (
6e55df1)onClickreceives a third argument,{ handleEdit }.ElementreceiveshandleEditandcanEditas props, so anElementthat renders its own<button>(with noonClick, as the README advises) can commit too.handleEdit(value)commits a new value at the button's own node, the same way an edit does: throughonUpdate, theonEditEventstream, and the node's inline error andonErroron rejection. The value can be any type, so a string can become an object.allowEdit.canEditlets a button hide itself on read-only nodes if the consumer wants.handleEdit(value), and a collection row goes through its ownhandleEdit(see 2).onClickis optional in the type, matching the README and the implementation.CustomButtonElementPropsis exported.The name is
handleEditrather thansetValuebecause on value componentssetValueonly writes to the edit buffer, whilehandleEdit(value)commits, which is what a button does.BooleanToggle, the closest existing analogue to a one-click button, already commits withhandleEdit(value).2. Custom collection components (
3274e06)handleEdittakes an optional value, as a value node's does:handleEdit(value)commits that value, andhandleEdit()commits the raw-JSON buffer as before.setValuekeeps committing, so existing components are unaffected, but it now runs throughhandleEdit(value). A rejection therefore shows the node's inline error and reachesonError, where before it fired onlyupdateError.setValuebuffer-only on collections, to match value components, would be breaking, so it's a TO-DO for v3 in the type comment.Docs and demo
handleEdit,canEditand an example. The custom-component props list has separate entries forhandleEdit(newValue)andsetValue(newValue), and the collection section says to commit withhandleEdit(newValue).2.0.4. These add features, so2.1.0may be the better version number.onUpdatetoast that shows which changes pass through it. Duplicate is unchanged (it inserts into the parent, which a node-boundhandleEditcan't do), with a comment explaining why it usessetData.handleEdit.Note
The demo pins
json-edit-react@2.0.3. When it's built against npm, both examples need the new core: the read toggle throws and the playlist buttons commit nothing until the dependency is bumped after release.Test plan
test/customButtons.test.tsx(10 tests): commits on value and collection rows, a value row becoming an object, thesubmitEdit→commitEdit→updateSuccesssequence, sync rejection on both row kinds, async rejection and revert,canEditandallowEdit, anElementwith its own button, and a button that ignores the argument. All failed before the change.test/customNode.test.tsx(5): a collection component committing viahandleEdit(value)and viasetValue, inside its own edit session, and rejection reporting for both. Four failed before the change; thesetValuecommit test passed before and after, as intended.pnpm test,pnpm lint,pnpm compile,SKIP_TESTS=1 pnpm build(tree-shake guard), and the demo typecheck all pass.onUpdatetoast, and the playlist's Add track, Reorder → Reverse (the toolbar stays open), Done, and Edit as JSON → ✓ all work.🤖 Generated with Claude Code