fix(ToggleButton): pass click event to onPressedChange for preventDefault opt-out#3160
Merged
Merged
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cixzhang
force-pushed
the
navi/fix/togglebutton-onpressedchange-event
branch
2 times, most recently
from
75a64bf to
d559ef6
Compare
cixzhang
changed the base branch from
navi/fix/togglebutton-pressedchangeaction-transition
to
main
cixzhang
requested review from
ejhammond,
imdreamrunner,
josephfarina,
marie-lucas,
nynexman4464 and
thedjpetersen
as code owners
Contributor
🚀 Vercel Preview Deployment
|
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
josephfarina
approved these changes
Jun 26, 2026
cixzhang
commented
Jun 26, 2026
Comment on lines
+284
to
+290
| startTransition(async () => { | ||
| setOptimisticPressed(newState); | ||
| onPressedChangeProp?.(newState, event); | ||
| if (!event.defaultPrevented) { | ||
| await pressedChangeAction?.(newState); | ||
| } | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
Avoid extra startTransition out in ToggleButton. Instead make use of Button's clickAction
Replace the spinner debounce (useDelayed) with Button's new isInterruptible prop. The pending spinner now shows immediately but no longer disables the button, so re-clicks always land and interrupt the in-flight action instead of racing a 150ms debounce window. Adds Button.isInterruptible: loading still renders the spinner and aria-busy but does not disable when set. ToggleButton only treats a transition as pending when an async pressedChangeAction is present, so a purely local toggle does not flash a spinner.
…ault opt-out onPressedChange now receives the originating click event as a second argument. Calling event.preventDefault() skips pressedChangeAction, so a consumer can fully handle the toggle in onPressedChange without firing the action — matching how Switch's onChange and Button's onClick gate their action props. The event is an added trailing argument, so existing (isPressed) => void handlers are unaffected.
A fast clickAction (or an interruptible caller like ToggleButton) that settles quickly no longer flashes a spinner: the spinner reveal and content hide share a CSS animation-delay, so the swap only happens if the action stays pending past the delay. The delay is purely visual — aria-busy and the disabled state are set synchronously — and is removed under prefers-reduced-motion. Explicit isLoading stays immediate.
…; drop Button isInterruptible ToggleButton no longer runs its own useTransition. The synchronous part (onPressedChange + preventDefault opt-out, group mode, disabled) runs in Button's onClick, and pressedChangeAction runs through Button's clickAction, so Button owns the pending state and spinner. Removes the now-unused Button isInterruptible prop. Drops the interrupt-on-re-click behavior, which Button's fire-once clickAction guard does not provide.
…ruptible Reintroduce Button's isInterruptible prop and wire it into the clickAction path: when set, the loading state drives the spinner and aria-busy but does not disable the button, and the fire-once ref guard is bypassed so a re-click lands and interrupts the in-flight action with a fresh one. ToggleButton renders Button interruptible and derives the next pressed state from the optimistic value (not the committed one), so rapid re-clicks toggle true -> false -> true while an action is pending instead of stalling.
cixzhang
force-pushed
the
navi/fix/togglebutton-onpressedchange-event
branch
from
a2bf69e to
ef266b6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's in here
1.
Button.isInterruptible(new prop)When set, the loading state still renders the spinner and
aria-busy, but the button is not disabled — so clicks keep landing. Defaults tofalse, so existingisLoadingbehavior (spinner + disabled) is unchanged.2. Button: CSS spinner delay for action-driven loading
A fast
clickAction(or an interruptible caller likeToggleButton) that settles quickly no longer flashes a spinner. The spinner reveal and content hide share a CSSanimation-delay, so the loading swap only becomes visible if the action stays pending past the delay. This replaces the earlier JS debounce (useDelayed+ a 150ms timer) with pure CSS at the Button layer, so it covers bothclickActionand the toggle case.aria-busyand the disabled state are set synchronously, so accessibility/DOM state is never delayed.prefers-reduced-motion.isLoading(controlled, neither pending nor interruptible) stays immediate — the consumer is deliberately showing it.3. ToggleButton: interruptible while pending
Drops the JS spinner-debounce hack and relies on
Button.isInterruptible+ the CSS delay. The pending spinner no longer disables the button, so re-clicks always land and interrupt the in-flight action (e.g.true → false → true). The spinner is also suppressed for purely-local toggles (no async action), so a plain toggle never flashes one.4. ToggleButton:
preventDefaultopt-out ononPressedChangeonPressedChangenow receives the originating click event as a second argument:Calling
event.preventDefault()skipspressedChangeAction, so a consumer can handle the toggle entirely in the callback. Mirrors theSwitch.onChange/Button.onClickconvention. The event is an added trailing argument, so existing(isPressed) => voidhandlers — including all current stories — are unaffected.Test plan
isInterruptibleshowsaria-busybut keeps the button clickable;clickActionsetsaria-busy/disabled synchronously while pending and clears on settle (locking the "delay is visual-only" contract); defaultisLoadingstill disables.true → false → true); sync action supported;preventDefaultskips the action.Review feedback (from #3056)
preventDefault()— done.Button.isInterruptible.animation-delayinstead of a JS timer — done at the Button layer (coversclickActiontoo).startTransition+ deriveisPendingfrom optimistic state — keptuseTransitiondeliberately: it re-throws action errors (isomorphic doesn't), and optimistic-derivedisPendingwould read "not pending" on afalse → true → falsemid-flight toggle.