diff --git a/.changeset/toggleswitch-onchange-from-handler.md b/.changeset/toggleswitch-onchange-from-handler.md new file mode 100644 index 00000000000..c2fc448c00d --- /dev/null +++ b/.changeset/toggleswitch-onchange-from-handler.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +ToggleSwitch: Fire `onChange` from the user interaction instead of an effect, so it no longer fires on mount or when a controlled `checked` value changes externally. diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx index bdcbdc45ab1..f665ff6d1dc 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx @@ -181,6 +181,46 @@ describe('ToggleSwitch', () => { expect(handleChange).toHaveBeenCalledWith(true) }) + it('does not call onChange on mount or when checked changes externally', () => { + const handleChange = vi.fn() + const {rerender} = render( + <> +
{SWITCH_LABEL_TEXT}
+ + , + ) + expect(handleChange).not.toHaveBeenCalled() + + rerender( + <> +
{SWITCH_LABEL_TEXT}
+ + , + ) + expect(handleChange).not.toHaveBeenCalled() + }) + + it('does not call onChange when a new inline onChange is passed on rerender', () => { + const handleChange = vi.fn() + const {rerender} = render( + <> +
{SWITCH_LABEL_TEXT}
+ handleChange()} aria-labelledby="switchLabel" /> + , + ) + expect(handleChange).not.toHaveBeenCalled() + + // A new `onChange` identity each render previously re-ran the effect and + // fired the callback; it must not be called without a user interaction. + rerender( + <> +
{SWITCH_LABEL_TEXT}
+ handleChange()} aria-labelledby="switchLabel" /> + , + ) + expect(handleChange).not.toHaveBeenCalled() + }) + it('can pass data attributes to the rendered component', async () => { const TEST_ID = 'a test id' const ControlledSwitchComponent = () => { diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx index ebe8e7fc512..156586378c9 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx @@ -109,20 +109,21 @@ const ToggleSwitch = React.forwardRef(func e => { if (disabled || loading) return - if (!isControlled) { + if (isControlled) { + // For controlled usage the click is the source of the change, so notify the + // consumer here rather than echoing the `checked` prop back from an effect + // (which fired on mount and whenever any of its dependencies — `checked`, + // `disabled`, or `onChange` — changed). + onChange?.(!isOn) + } else { + // `setIsOn` notifies `onChange` for uncontrolled usage. setIsOn(!isOn) } onClick && onClick(e) }, - [disabled, isControlled, loading, onClick, setIsOn, isOn], + [disabled, isControlled, loading, onClick, onChange, setIsOn, isOn], ) - useEffect(() => { - if (onChange && isControlled && !disabled) { - onChange(Boolean(checked)) - } - }, [onChange, checked, isControlled, disabled]) - useEffect(() => { if (!loading && isLoadingLabelVisible) { // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-chain-state-updates