From 25a52184c37b979d0d2f27a4f829e2780104b1e5 Mon Sep 17 00:00:00 2001 From: logonoff Date: Wed, 23 Jul 2025 16:22:29 -0400 Subject: [PATCH 1/2] fix: improve type consistency in `onSelect` callback --- packages/react-core/src/components/Dropdown/Dropdown.tsx | 3 ++- packages/react-core/src/components/Menu/Menu.tsx | 3 ++- packages/react-core/src/components/Select/Select.tsx | 3 ++- packages/react-core/src/components/Tabs/Tabs.tsx | 4 ++-- .../src/components/Dropdown/SimpleDropdown.tsx | 2 +- .../react-templates/src/components/Select/CheckboxSelect.tsx | 2 +- .../react-templates/src/components/Select/SimpleSelect.tsx | 2 +- .../react-templates/src/components/Select/TypeaheadSelect.tsx | 2 +- 8 files changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/react-core/src/components/Dropdown/Dropdown.tsx b/packages/react-core/src/components/Dropdown/Dropdown.tsx index 9ad39d49477..af4ed516543 100644 --- a/packages/react-core/src/components/Dropdown/Dropdown.tsx +++ b/packages/react-core/src/components/Dropdown/Dropdown.tsx @@ -2,6 +2,7 @@ import { forwardRef, useEffect, useRef } from 'react'; import { css } from '@patternfly/react-styles'; import { Menu, MenuContent, MenuProps } from '../Menu'; import { Popper } from '../../helpers/Popper/Popper'; +import type { DropdownItemProps } from './DropdownItem'; import { useOUIAProps, OUIAProps, onToggleArrowKeydownDefault } from '../../helpers'; export interface DropdownPopperProps { @@ -45,7 +46,7 @@ export interface DropdownProps extends MenuProps, OUIAProps { /** Flag indicating the toggle should be focused after a selection. If this use case is too restrictive, the optional toggleRef property with a node toggle may be used to control focus. */ shouldFocusToggleOnSelect?: boolean; /** Function callback called when user selects item. */ - onSelect?: (event?: React.MouseEvent, value?: string | number) => void; + onSelect?: (event?: React.MouseEvent, value?: DropdownItemProps['value']) => void; /** Callback to allow the dropdown component to change the open state of the menu. * Triggered by clicking outside of the menu, or by pressing any keys specified in onOpenChangeKeys. */ onOpenChange?: (isOpen: boolean) => void; diff --git a/packages/react-core/src/components/Menu/Menu.tsx b/packages/react-core/src/components/Menu/Menu.tsx index 2194774b104..02ecf7df509 100644 --- a/packages/react-core/src/components/Menu/Menu.tsx +++ b/packages/react-core/src/components/Menu/Menu.tsx @@ -4,6 +4,7 @@ import breadcrumbStyles from '@patternfly/react-styles/css/components/Breadcrumb import { css } from '@patternfly/react-styles'; import { getOUIAProps, OUIAProps, getDefaultOUIAId } from '../../helpers'; import { MenuContext } from './MenuContext'; +import type { MenuItemProps } from './MenuItem'; import { canUseDOM } from '../../helpers/util'; import { KeyboardHandler } from '../../helpers'; export interface MenuProps extends Omit, 'ref' | 'onSelect'>, OUIAProps { @@ -14,7 +15,7 @@ export interface MenuProps extends Omit, 'r /** ID of the menu */ id?: string; /** Callback for updating when item selection changes. You can also specify onClick on the MenuItem. */ - onSelect?: (event?: React.MouseEvent, itemId?: string | number) => void; + onSelect?: (event?: React.MouseEvent, itemId?: MenuItemProps['itemId']) => void; /** Single itemId for single select menus, or array of itemIds for multi select. You can also specify isSelected on the MenuItem. */ selected?: any | any[]; /** Callback called when an MenuItems's action button is clicked. You can also specify it within a MenuItemAction. */ diff --git a/packages/react-core/src/components/Select/Select.tsx b/packages/react-core/src/components/Select/Select.tsx index d42ec485eca..8b597aec153 100644 --- a/packages/react-core/src/components/Select/Select.tsx +++ b/packages/react-core/src/components/Select/Select.tsx @@ -3,6 +3,7 @@ import { css } from '@patternfly/react-styles'; import { Menu, MenuContent, MenuProps } from '../Menu'; import { Popper } from '../../helpers/Popper/Popper'; import { getOUIAProps, OUIAProps, getDefaultOUIAId, onToggleArrowKeydownDefault } from '../../helpers'; +import type { SelectOptionProps } from './SelectOption'; export interface SelectPopperProps { /** Vertical direction of the popper. If enableFlip is set to true, this will set the initial direction before the popper flips. */ @@ -56,7 +57,7 @@ export interface SelectProps extends MenuProps, OUIAProps { /** @beta Flag indicating the first menu item should be focused after opening the menu. */ shouldFocusFirstItemOnOpen?: boolean; /** Function callback when user selects an option. */ - onSelect?: (event?: React.MouseEvent, value?: string | number) => void; + onSelect?: (event?: React.MouseEvent, value?: SelectOptionProps['value']) => void; /** Callback to allow the select component to change the open state of the menu. * Triggered by clicking outside of the menu, or by pressing any keys specified in onOpenChangeKeys. */ onOpenChange?: (isOpen: boolean) => void; diff --git a/packages/react-core/src/components/Tabs/Tabs.tsx b/packages/react-core/src/components/Tabs/Tabs.tsx index 12a75bd62e7..b5a7a7a78e1 100644 --- a/packages/react-core/src/components/Tabs/Tabs.tsx +++ b/packages/react-core/src/components/Tabs/Tabs.tsx @@ -54,9 +54,9 @@ export interface TabsProps /** The index of the default active tab. Set this for uncontrolled Tabs */ defaultActiveKey?: number | string; /** Callback to handle tab selection */ - onSelect?: (event: React.MouseEvent, eventKey: number | string) => void; + onSelect?: (event: React.MouseEvent, eventKey: TabProps['eventKey']) => void; /** Callback to handle tab closing and adds a basic close button to all tabs. This is overridden by the tab actions property. */ - onClose?: (event: React.MouseEvent, eventKey: number | string) => void; + onClose?: (event: React.MouseEvent, eventKey: TabProps['eventKey']) => void; /** Callback for the add button. Passing this property inserts the add button */ onAdd?: (event: React.MouseEvent) => void; /** Aria-label for the add button */ diff --git a/packages/react-templates/src/components/Dropdown/SimpleDropdown.tsx b/packages/react-templates/src/components/Dropdown/SimpleDropdown.tsx index 3fa420ea155..9b628fd1778 100644 --- a/packages/react-templates/src/components/Dropdown/SimpleDropdown.tsx +++ b/packages/react-templates/src/components/Dropdown/SimpleDropdown.tsx @@ -35,7 +35,7 @@ export interface SimpleDropdownProps extends Omit, value?: string | number) => void; + onSelect?: (event?: React.MouseEvent, value?: SimpleDropdownItem['value']) => void; /** Callback triggered when the dropdown toggle opens or closes. */ onToggle?: (nextIsOpen: boolean) => void; /** Flag indicating the dropdown toggle should be focused after a dropdown item is clicked. */ diff --git a/packages/react-templates/src/components/Select/CheckboxSelect.tsx b/packages/react-templates/src/components/Select/CheckboxSelect.tsx index b4a4bcf91a8..0280320e853 100644 --- a/packages/react-templates/src/components/Select/CheckboxSelect.tsx +++ b/packages/react-templates/src/components/Select/CheckboxSelect.tsx @@ -22,7 +22,7 @@ export interface CheckboxSelectProps extends Omit, value?: string | number) => void; + onSelect?: (_event: React.MouseEvent, value?: CheckboxSelectOption['value']) => void; /** Callback triggered when the select opens or closes. */ onToggle?: (nextIsOpen: boolean) => void; /** Flag indicating the select should be disabled. */ diff --git a/packages/react-templates/src/components/Select/SimpleSelect.tsx b/packages/react-templates/src/components/Select/SimpleSelect.tsx index 44e498746e1..6428b687fa0 100644 --- a/packages/react-templates/src/components/Select/SimpleSelect.tsx +++ b/packages/react-templates/src/components/Select/SimpleSelect.tsx @@ -21,7 +21,7 @@ export interface SimpleSelectProps extends Omit, selection: string | number) => void; + onSelect?: (_event: React.MouseEvent, selection: SimpleSelectOption['value']) => void; /** Callback triggered when the select opens or closes. */ onToggle?: (nextIsOpen: boolean) => void; /** Flag indicating the select should be disabled. */ diff --git a/packages/react-templates/src/components/Select/TypeaheadSelect.tsx b/packages/react-templates/src/components/Select/TypeaheadSelect.tsx index 29a999baf16..0e37660ba4f 100644 --- a/packages/react-templates/src/components/Select/TypeaheadSelect.tsx +++ b/packages/react-templates/src/components/Select/TypeaheadSelect.tsx @@ -30,7 +30,7 @@ export interface TypeaheadSelectProps extends Omit | React.KeyboardEvent | undefined, - selection: string | number + selection: TypeaheadSelectOption['value'] ) => void; /** Callback triggered when the select opens or closes. */ onToggle?: (nextIsOpen: boolean) => void; From 599d52549bb0945538ddd50c00678d9a58f37359 Mon Sep 17 00:00:00 2001 From: logonoff Date: Wed, 30 Jul 2025 13:18:33 -0400 Subject: [PATCH 2/2] feat: add unit test for Menu --- .../components/Menu/__tests__/Menu.test.tsx | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx b/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx index 44bcb2c0fb4..08b9ffcbdec 100644 --- a/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx +++ b/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx @@ -2,7 +2,7 @@ import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; import { Menu } from '../Menu'; -import { MenuItem } from '../MenuItem'; +import { MenuItem, MenuItemProps } from '../MenuItem'; import { MenuList } from '../MenuList'; import { MenuContent } from '../MenuContent'; @@ -16,6 +16,44 @@ describe('Menu', () => { expect(asFragment()).toMatchSnapshot(); }); + test('should accept onSelect with various types', () => { + const onSelectMock = jest.fn(); + + const items: MenuItemProps[] = [ + { itemId: 1, children: 'Item 1', key: 'item1' }, + { itemId: new Date(1), children: 'Item 2', key: 'item2' }, + { itemId: 'item3', children: 'Item 3', key: 'item3' }, + { itemId: { some: 'object' }, children: 'Item 4', key: 'item4' }, + { itemId: NaN, children: 'Item 5', key: 'item5' }, + { itemId: null, children: 'Item 6', key: 'item6' }, + { itemId: 0n, children: 'Item 7', key: 'item7' }, + { itemId: true, children: 'Item 8', key: 'item8' }, + { itemId: false, children: 'Item 9', key: 'item9' }, + { itemId: -0, children: 'Item 10', key: 'item10' }, + { itemId: '', children: 'Item 11', key: 'item11' } + ]; + + render( + + + + {items.map((item) => ( + + {item.children} + + ))} + + + + ); + + for (const item of items) { + const menuItem = screen.getByText(item.children as string); + menuItem.click(); + expect(onSelectMock).toHaveBeenCalledWith(expect.anything(), item.itemId); + } + }); + describe('with isPlain', () => { test('should render Menu with plain styles applied', () => { render(