Skip to content

Commit d50ba6d

Browse files
logonofflogonoff
authored
fix(Select): improve type consistency in onSelect (#11935)
* fix: improve type consistency in `onSelect` callback * feat: add unit test for Menu
1 parent 371e6ef commit d50ba6d

9 files changed

Lines changed: 51 additions & 10 deletions

File tree

‎packages/react-core/src/components/Dropdown/Dropdown.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { forwardRef, useEffect, useRef } from 'react';
22
import { css } from '@patternfly/react-styles';
33
import { Menu, MenuContent, MenuProps } from '../Menu';
44
import { Popper, PopperOptions } from '../../helpers/Popper/Popper';
5+
import type { DropdownItemProps } from './DropdownItem';
56
import { useOUIAProps, OUIAProps, onToggleArrowKeydownDefault } from '../../helpers';
67

78
/** @deprecated Use PopperOptions instead */
@@ -29,7 +30,7 @@ export interface DropdownProps extends MenuProps, OUIAProps {
2930
/** 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. */
3031
shouldFocusToggleOnSelect?: boolean;
3132
/** Function callback called when user selects item. */
32-
onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: string | number) => void;
33+
onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: DropdownItemProps['value']) => void;
3334
/** Callback to allow the dropdown component to change the open state of the menu.
3435
* Triggered by clicking outside of the menu, or by pressing any keys specified in onOpenChangeKeys. */
3536
onOpenChange?: (isOpen: boolean) => void;

‎packages/react-core/src/components/Menu/Menu.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import breadcrumbStyles from '@patternfly/react-styles/css/components/Breadcrumb
44
import { css } from '@patternfly/react-styles';
55
import { getOUIAProps, OUIAProps, getDefaultOUIAId } from '../../helpers';
66
import { MenuContext } from './MenuContext';
7+
import type { MenuItemProps } from './MenuItem';
78
import { canUseDOM } from '../../helpers/util';
89
import { KeyboardHandler } from '../../helpers';
910
export interface MenuProps extends Omit<React.HTMLAttributes<HTMLDivElement>, 'ref' | 'onSelect'>, OUIAProps {
@@ -14,7 +15,7 @@ export interface MenuProps extends Omit<React.HTMLAttributes<HTMLDivElement>, 'r
1415
/** ID of the menu */
1516
id?: string;
1617
/** Callback for updating when item selection changes. You can also specify onClick on the MenuItem. */
17-
onSelect?: (event?: React.MouseEvent, itemId?: string | number) => void;
18+
onSelect?: (event?: React.MouseEvent, itemId?: MenuItemProps['itemId']) => void;
1819
/** Single itemId for single select menus, or array of itemIds for multi select. You can also specify isSelected on the MenuItem. */
1920
selected?: any | any[];
2021
/** 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‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { render, screen } from '@testing-library/react';
22
import '@testing-library/jest-dom';
33

44
import { Menu } from '../Menu';
5-
import { MenuItem } from '../MenuItem';
5+
import { MenuItem, MenuItemProps } from '../MenuItem';
66
import { MenuList } from '../MenuList';
77
import { MenuContent } from '../MenuContent';
88

@@ -16,6 +16,44 @@ describe('Menu', () => {
1616
expect(asFragment()).toMatchSnapshot();
1717
});
1818

19+
test('should accept onSelect with various types', () => {
20+
const onSelectMock = jest.fn();
21+
22+
const items: MenuItemProps[] = [
23+
{ itemId: 1, children: 'Item 1', key: 'item1' },
24+
{ itemId: new Date(1), children: 'Item 2', key: 'item2' },
25+
{ itemId: 'item3', children: 'Item 3', key: 'item3' },
26+
{ itemId: { some: 'object' }, children: 'Item 4', key: 'item4' },
27+
{ itemId: NaN, children: 'Item 5', key: 'item5' },
28+
{ itemId: null, children: 'Item 6', key: 'item6' },
29+
{ itemId: 0n, children: 'Item 7', key: 'item7' },
30+
{ itemId: true, children: 'Item 8', key: 'item8' },
31+
{ itemId: false, children: 'Item 9', key: 'item9' },
32+
{ itemId: -0, children: 'Item 10', key: 'item10' },
33+
{ itemId: '', children: 'Item 11', key: 'item11' }
34+
];
35+
36+
render(
37+
<Menu activeItemId={0} onSelect={onSelectMock}>
38+
<MenuContent>
39+
<MenuList>
40+
{items.map((item) => (
41+
<MenuItem key={item.key} itemId={item.itemId}>
42+
{item.children}
43+
</MenuItem>
44+
))}
45+
</MenuList>
46+
</MenuContent>
47+
</Menu>
48+
);
49+
50+
for (const item of items) {
51+
const menuItem = screen.getByText(item.children as string);
52+
menuItem.click();
53+
expect(onSelectMock).toHaveBeenCalledWith(expect.anything(), item.itemId);
54+
}
55+
});
56+
1957
describe('with isPlain', () => {
2058
test('should render Menu with plain styles applied', () => {
2159
render(

‎packages/react-core/src/components/Select/Select.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { css } from '@patternfly/react-styles';
33
import { Menu, MenuContent, MenuProps } from '../Menu';
44
import { Popper, PopperOptions } from '../../helpers/Popper/Popper';
55
import { getOUIAProps, OUIAProps, getDefaultOUIAId, onToggleArrowKeydownDefault } from '../../helpers';
6+
import type { SelectOptionProps } from './SelectOption';
67

78
/** @deprecated Use PopperOptions instead */
89
export type SelectPopperProps = PopperOptions;
@@ -34,7 +35,7 @@ export interface SelectProps extends MenuProps, OUIAProps {
3435
/** @beta Flag indicating the first menu item should be focused after opening the menu. */
3536
shouldFocusFirstItemOnOpen?: boolean;
3637
/** Function callback when user selects an option. */
37-
onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: string | number) => void;
38+
onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: SelectOptionProps['value']) => void;
3839
/** Callback to allow the select component to change the open state of the menu.
3940
* Triggered by clicking outside of the menu, or by pressing any keys specified in onOpenChangeKeys. */
4041
onOpenChange?: (isOpen: boolean) => void;

‎packages/react-core/src/components/Tabs/Tabs.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,9 @@ export interface TabsProps
5454
/** The index of the default active tab. Set this for uncontrolled Tabs */
5555
defaultActiveKey?: number | string;
5656
/** Callback to handle tab selection */
57-
onSelect?: (event: React.MouseEvent<HTMLElement, MouseEvent>, eventKey: number | string) => void;
57+
onSelect?: (event: React.MouseEvent<HTMLElement, MouseEvent>, eventKey: TabProps['eventKey']) => void;
5858
/** Callback to handle tab closing and adds a basic close button to all tabs. This is overridden by the tab actions property. */
59-
onClose?: (event: React.MouseEvent<HTMLElement, MouseEvent>, eventKey: number | string) => void;
59+
onClose?: (event: React.MouseEvent<HTMLElement, MouseEvent>, eventKey: TabProps['eventKey']) => void;
6060
/** Callback for the add button. Passing this property inserts the add button */
6161
onAdd?: (event: React.MouseEvent<HTMLElement, MouseEvent>) => void;
6262
/** Aria-label for the add button */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ export interface SimpleDropdownProps extends Omit<DropdownProps, 'toggle' | 'onT
3535
/** Flag indicated whether the dropdown toggle should take up the full width of its parent. */
3636
isToggleFullWidth?: boolean;
3737
/** Callback triggered when any dropdown item is clicked. */
38-
onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: string | number) => void;
38+
onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: SimpleDropdownItem['value']) => void;
3939
/** Callback triggered when the dropdown toggle opens or closes. */
4040
onToggle?: (nextIsOpen: boolean) => void;
4141
/** Flag indicating the dropdown toggle should be focused after a dropdown item is clicked. */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export interface CheckboxSelectProps extends Omit<SelectProps, 'toggle' | 'onTog
2222
/** Initial options of the select. */
2323
initialOptions?: CheckboxSelectOption[];
2424
/** Callback triggered on selection. */
25-
onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, value?: string | number) => void;
25+
onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, value?: CheckboxSelectOption['value']) => void;
2626
/** Callback triggered when the select opens or closes. */
2727
onToggle?: (nextIsOpen: boolean) => void;
2828
/** Flag indicating the select should be disabled. */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export interface SimpleSelectProps extends Omit<SelectProps, 'toggle' | 'onToggl
2121
/** Initial options of the select. */
2222
initialOptions?: SimpleSelectOption[];
2323
/** Callback triggered on selection. */
24-
onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, selection: string | number) => void;
24+
onSelect?: (_event: React.MouseEvent<Element, MouseEvent>, selection: SimpleSelectOption['value']) => void;
2525
/** Callback triggered when the select opens or closes. */
2626
onToggle?: (nextIsOpen: boolean) => void;
2727
/** Flag indicating the select should be disabled. */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export interface TypeaheadSelectProps extends Omit<SelectProps, 'toggle' | 'onSe
3030
/** Callback triggered on selection. */
3131
onSelect?: (
3232
_event: React.MouseEvent<Element, MouseEvent> | React.KeyboardEvent<HTMLInputElement> | undefined,
33-
selection: string | number
33+
selection: TypeaheadSelectOption['value']
3434
) => void;
3535
/** Callback triggered when the select opens or closes. */
3636
onToggle?: (nextIsOpen: boolean) => void;

0 commit comments

Comments
 (0)