fix(Select): improve type consistency in onSelect (#11935)

* fix: improve type consistency in `onSelect` callback

* feat: add unit test for Menu

Author: logonoff

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, PopperOptions } from '../../helpers/Popper/Popper';
+import type { DropdownItemProps } from './DropdownItem';
 import { useOUIAProps, OUIAProps, onToggleArrowKeydownDefault } from '../../helpers';
 
 /** @deprecated Use PopperOptions instead */
@@ -29,7 +30,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<Element, MouseEvent>, value?: string | number) => void;
+  onSelect?: (event?: React.MouseEvent<Element, 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;

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<React.HTMLAttributes<HTMLDivElement>, 'ref' | 'onSelect'>, OUIAProps {
@@ -14,7 +15,7 @@ export interface MenuProps extends Omit<React.HTMLAttributes<HTMLDivElement>, '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. */

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(
+      <Menu activeItemId={0} onSelect={onSelectMock}>
+        <MenuContent>
+          <MenuList>
+            {items.map((item) => (
+              <MenuItem key={item.key} itemId={item.itemId}>
+                {item.children}
+              </MenuItem>
+            ))}
+          </MenuList>
+        </MenuContent>
+      </Menu>
+    );
+
+    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(

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, PopperOptions } from '../../helpers/Popper/Popper';
 import { getOUIAProps, OUIAProps, getDefaultOUIAId, onToggleArrowKeydownDefault } from '../../helpers';
+import type { SelectOptionProps } from './SelectOption';
 
 /** @deprecated Use PopperOptions instead */
 export type SelectPopperProps = PopperOptions;
@@ -34,7 +35,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<Element, MouseEvent>, value?: string | number) => void;
+  onSelect?: (event?: React.MouseEvent<Element, 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;

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<HTMLElement, MouseEvent>, eventKey: number | string) => void;
+  onSelect?: (event: React.MouseEvent<HTMLElement, 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<HTMLElement, MouseEvent>, eventKey: number | string) => void;
+  onClose?: (event: React.MouseEvent<HTMLElement, MouseEvent>, eventKey: TabProps['eventKey']) => void;
   /** Callback for the add button. Passing this property inserts the add button */
   onAdd?: (event: React.MouseEvent<HTMLElement, MouseEvent>) => void;
   /** Aria-label for the add button */

packages/react-templates/src/components/Dropdown/SimpleDropdown.tsx

@@ -35,7 +35,7 @@ export interface SimpleDropdownProps extends Omit<DropdownProps, 'toggle' | 'onT
   /** Flag indicated whether the dropdown toggle should take up the full width of its parent. */
   isToggleFullWidth?: boolean;
   /** Callback triggered when any dropdown item is clicked. */
-  onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: string | number) => void;
+  onSelect?: (event?: React.MouseEvent<Element, 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. */

packages/react-templates/src/components/Select/CheckboxSelect.tsx

@@ -22,7 +22,7 @@ export interface CheckboxSelectProps extends Omit<SelectProps, 'toggle' | 'onTog
   /** Initial options of the select. */
   initialOptions?: CheckboxSelectOption[];
   /** Callback triggered on selection. */
-  onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, value?: string | number) => void;
+  onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, value?: CheckboxSelectOption['value']) => void;
   /** Callback triggered when the select opens or closes. */
   onToggle?: (nextIsOpen: boolean) => void;
   /** Flag indicating the select should be disabled. */

packages/react-templates/src/components/Select/SimpleSelect.tsx

@@ -21,7 +21,7 @@ export interface SimpleSelectProps extends Omit<SelectProps, 'toggle' | 'onToggl
   /** Initial options of the select. */
   initialOptions?: SimpleSelectOption[];
   /** Callback triggered on selection. */
-  onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, selection: string | number) => void;
+  onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, selection: SimpleSelectOption['value']) => void;
   /** Callback triggered when the select opens or closes. */
   onToggle?: (nextIsOpen: boolean) => void;
   /** Flag indicating the select should be disabled. */

packages/react-templates/src/components/Select/TypeaheadSelect.tsx

@@ -30,7 +30,7 @@ export interface TypeaheadSelectProps extends Omit<SelectProps, 'toggle' | 'onSe
   /** Callback triggered on selection. */
   onSelect?: (
     _event: React.MouseEvent<Element, MouseEvent> | React.KeyboardEvent<HTMLInputElement> | undefined,
-    selection: string | number
+    selection: TypeaheadSelectOption['value']
   ) => void;
   /** Callback triggered when the select opens or closes. */
   onToggle?: (nextIsOpen: boolean) => void;