Skip to content

Let custom buttons and collection components commit with handleEdit (#418) - #421

Merged
CarlosNZ merged 4 commits into
mainfrom
418-custom-button-enhancement
Oct 3, 2026
Merged

CarlosNZ merged 4 commits into
mainfrom
418-custom-button-enhancement

Conversation

@CarlosNZ

@CarlosNZ CarlosNZ commented Oct 3, 2026

Copy link
Copy Markdown
Owner

Closes #418.

Summary

A custom button had no way to commit a change through the edit pipeline: its onClick received only nodeData and the event, so it had to call the host's setData, which skips onUpdate and onEditEvent. This PR gives buttons a handleEdit, and makes handleEdit(value) the commit call for custom collection components too.

1. Custom buttons (6e55df1)

  • onClick receives a third argument, { handleEdit }. Element receives handleEdit and canEdit as props, so an Element that renders its own <button> (with no onClick, 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: through onUpdate, the onEditEvent stream, and the node's inline error and onError on rejection. The value can be any type, so a string can become an object.
  • It isn't gated by allowEdit. canEdit lets a button hide itself on read-only nodes if the consumer wants.
  • Each node supplies the function, so a rejection settles on that node: a value row reuses its existing handleEdit(value), and a collection row goes through its own handleEdit (see 2).
  • onClick is optional in the type, matching the README and the implementation. CustomButtonElementProps is exported.

The name is handleEdit rather than setValue because on value components setValue only writes to the edit buffer, while handleEdit(value) commits, which is what a button does. BooleanToggle, the closest existing analogue to a one-click button, already commits with handleEdit(value).

2. Custom collection components (3274e06)

  • A collection's handleEdit takes an optional value, as a value node's does: handleEdit(value) commits that value, and handleEdit() commits the raw-JSON buffer as before.
  • A collection component's setValue keeps committing, so existing components are unaffected, but it now runs through handleEdit(value). A rejection therefore shows the node's inline error and reaches onError, where before it fired only updateError.
  • Making setValue buffer-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

  • README: the Custom buttons section covers handleEdit, canEdit and an example. The custom-component props list has separate entries for handleEdit(newValue) and setValue(newValue), and the collection section says to commit with handleEdit(newValue).
  • CHANGELOG: two bullets under 2.0.4. These add features, so 2.1.0 may be the better version number.
  • Demo, custom-buttons example: adds a "Toggle read" button on each article, plus an onUpdate toast that shows which changes pass through it. Duplicate is unchanged (it inserts into the parent, which a node-bound handleEdit can't do), with a comment explaining why it uses setData.
  • Demo, playlist example: commits with 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

  • New test/customButtons.test.tsx (10 tests): commits on value and collection rows, a value row becoming an object, the submitEdit → commitEdit → updateSuccess sequence, sync rejection on both row kinds, async rejection and revert, canEdit and allowEdit, an Element with its own button, and a button that ignores the argument. All failed before the change.
  • New tests in test/customNode.test.tsx (5): a collection component committing via handleEdit(value) and via setValue, inside its own edit session, and rejection reporting for both. Four failed before the change; the setValue commit 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.
  • In Chrome against local source: the custom-buttons toggle round-trips and fires the onUpdate toast, and the playlist's Add track, Reorder → Reverse (the toolbar stays open), Done, and Edit as JSON → ✓ all work.

🤖 Generated with Claude Code

CarlosNZ and others added 2 commits October 4, 2026 01:08
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>
@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Bundle size impact

json-edit-react

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 CarlosNZ left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and commitValue (CollectionNode.tsx#L331) are the same one-argument wrapper. commitValue in both files would be clearer.
  • Two types for handleEdit: CustomComponentProps.handleEdit is (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 from onCommit to inputValue. Every internal caller is updated: commitOp passes (undefined, onCommit), handleKeyPress calls action() with no arguments, and click or native events are skipped by isJsEvent. None of the @json-edit-react/components components call a collection's handleEdit with an argument.
  • The type changes don't break existing code. A consumer's Element typed as FC<{ nodeData }> still fits the wider props type, and making onClick optional only affects code that calls def.onClick directly.
  • No performance cost. EditButtons isn't memoised, and the new handlers are recreated on each render, so they always read current state, as PERF-ARCHITECTURE requires.
  • The playlist flow (handleEdit(next) then setIsEditing(true)) works with no onUpdate, a sync one, an async one, and with hold(). 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

CarlosNZ and others added 2 commits October 4, 2026 11:43
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>
@CarlosNZ

CarlosNZ commented Oct 3, 2026

Copy link
Copy Markdown
Owner Author

Thanks for the review. Responses below; the changes are in 771ad63 and 422a48e.

1. Hidden buttons can still be pressed. Fixed at the root, not in the consumer's onClick. Both custom-button wrappers (the labelled <button> and the unlabelled <div>) carry a jer-custom-button class, and the stylesheet has .jer-custom-button:empty { display: none }. An Element that returns null leaves its wrapper empty, so the wrapper drops out of the accessibility tree and can't be pressed. It also no longer takes a flex gap, which was spacing the icons out. Consumers still just return null, and the README's advice is now accurate. The new tests load the real stylesheet (Jest stubs it) and check the role query and computed display. In Chrome, the custom-buttons example's accessibility tree lists only the buttons that render, and a panel whose custom buttons all render nothing is the same width as one with only the built-ins.

2. onClick lacks canEdit. Done. The third argument is renamed context and carries { handleEdit, canEdit }, so onClick and Element receive the same data. After the fix for point 1 this isn't needed for safety, but it keeps the two entry points consistent and helps a button that's always visible but behaves differently on read-only rows.

3 and 5. Not addressed in this PR.

4. A value turned into a collection stays collapsed. Fixed. When an explicit handleEdit(value) turns a value node into a collection, the commit sends the same expand command as the type switch (#217). It's sent at the commit moment, through the engine's onCommit, so a rejected edit leaves no pending command to open a collection that reaches that path later. renderCollectionAsValue nodes, which already hold a collection, are excluded. There are tests for both cases.

Nits. All three done. The wrapper is commitValue in both nodes. CustomComponentProps.handleEdit is typed (value?: JsonData); JsonData includes unknown, so this is purely for readability. The button's version keeps the value required, because a bare handleEdit() would commit an edit buffer a button never has open. The collection CHANGELOG bullet is cut to a sentence and a link.

Existing engine bug. Reproduced, and filed as #422 with the two-part cause: callbacks cleared by an unrelated commit, and hold() overwriting the session. It includes a proposed fix for the first part and options for the second. It isn't a blocker for this PR.

🤖 Generated with Claude Code

@CarlosNZ
CarlosNZ merged commit df8cffc into main Oct 3, 2026
2 checks passed
@CarlosNZ
CarlosNZ deleted the 418-custom-button-enhancement branch October 3, 2026 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom buttons can't commit a change through onUpdate

1 participant