Skip to content

Commit

Permalink
fix(MenuItemSelectable): remove duplicate onChange (#17967)
Browse files Browse the repository at this point in the history
This is the same issue and fix that was previously addressed for
`MenuItemRadioGroup` in #17736.

Also added a test to cover this case, including checking the value
passed to the `onChange` prop as the redundant `onChange` being
removed here was passing the SyntheticEvent instead of the expected
boolean.

Co-authored-by: Nikhil Tomar <[email protected]>
Co-authored-by: Anna Wen <[email protected]>
  • Loading branch information
3 people authored Nov 11, 2024
1 parent c868a3d commit 664408d
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 5 deletions.
31 changes: 30 additions & 1 deletion packages/react/src/components/Menu/Menu-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ describe('MenuItem', () => {
});
});

it('should call onChange once', async () => {
it('should call MenuItemRadioGroup onChange once', async () => {
const onChange = jest.fn();

render(
Expand All @@ -278,4 +278,33 @@ describe('MenuItem', () => {
await userEvent.click(screen.getByTitle('Item 1'));
expect(onChange).toHaveBeenCalledTimes(1);
});

it('should call MenuItemSelectable onChange once with correct value', async () => {
const onChange = jest.fn();

const { rerender } = render(
<Menu open label="Menu">
<MenuItemSelectable
label="Item 1"
onChange={onChange}
selected={false}
/>
</Menu>
);

await userEvent.click(screen.getByTitle('Item 1'));
expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange).toHaveBeenCalledWith(true);

onChange.mockClear();
rerender(
<Menu open label="Menu">
<MenuItemSelectable label="Item 1" onChange={onChange} selected />
</Menu>
);

await userEvent.click(screen.getByTitle('Item 1'));
expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange).toHaveBeenCalledWith(false);
});
});
4 changes: 0 additions & 4 deletions packages/react/src/components/Menu/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -377,10 +377,6 @@ export const MenuItemSelectable = forwardRef<

function handleClick(e) {
setChecked(!checked);

if (onChange) {
onChange(e);
}
}

useEffect(() => {
Expand Down

0 comments on commit 664408d

Please sign in to comment.