From 91d79076009ffc6c0dfa22d69f272655b8e246a3 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Fri, 21 Oct 2022 18:34:19 -0400 Subject: [PATCH 01/12] fix(NavItem): disallow flyout and link props to both be defined on NavItem --- .../react-core/src/components/Nav/NavItem.tsx | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/packages/react-core/src/components/Nav/NavItem.tsx index 3e8c78a3abe..8107d1db666 100644 --- a/packages/react-core/src/components/Nav/NavItem.tsx +++ b/packages/react-core/src/components/Nav/NavItem.tsx @@ -7,15 +7,13 @@ import { useOUIAProps, OUIAProps } from '../../helpers'; import { Popper } from '../../helpers/Popper/Popper'; import AngleRightIcon from '@patternfly/react-icons/dist/esm/icons/angle-right-icon'; -export interface NavItemProps extends Omit, 'onClick'>, OUIAProps { +interface CommonNavItemProps extends Omit, 'onClick'>, OUIAProps { /** Content rendered inside the nav item. */ children?: React.ReactNode; /** Whether to set className on children when React.isValidElement(children) */ styleChildren?: boolean; /** Additional classes added to the nav item */ className?: string; - /** Target navigation link */ - to?: string; /** Flag indicating whether the item is active */ isActive?: boolean; /** Group identifier, will be returned with the onToggle and onSelect callback passed to the Nav component */ @@ -28,8 +26,6 @@ export interface NavItemProps extends Omit, ' onClick?: NavSelectClickHandler; /** Component used to render NavItems if React.isValidElement(children) is false */ component?: React.ReactNode; - /** Flyout of a nav item. This should be a Menu component. */ - flyout?: React.ReactElement; /** Callback when flyout is opened or closed */ onShowFlyout?: () => void; /** @beta Opt-in for updated popper that does not use findDOMNode. */ @@ -40,6 +36,22 @@ export interface NavItemProps extends Omit, ' ouiaSafe?: boolean; } +type ConditionalNavItemProps = + | { + /** Target navigation link */ + to?: string; + /** Flyout of a nav item. Disallowed if nav link is defined */ + flyout?: never; + } + | { + /** Target navigation link. Disallowed if flyout is defined */ + to?: never; + /** Flyout of a nav item. This should be a Menu component. */ + flyout?: React.ReactNode; + }; + +export type NavItemProps = CommonNavItemProps & ConditionalNavItemProps; + export const NavItem: React.FunctionComponent = ({ children, styleChildren = true, From 2020766e6499e2a58819b5ef95bc5c24516ef3ab Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Fri, 21 Oct 2022 18:43:08 -0400 Subject: [PATCH 02/12] docs(Navigation): remove links from NavItems with flyouts --- .../react-core/src/components/Nav/examples/NavFlyout.tsx | 1 - .../demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx | 8 +------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/react-core/src/components/Nav/examples/NavFlyout.tsx b/packages/react-core/src/components/Nav/examples/NavFlyout.tsx index 503b84fe9b0..59ae5d5e78f 100644 --- a/packages/react-core/src/components/Nav/examples/NavFlyout.tsx +++ b/packages/react-core/src/components/Nav/examples/NavFlyout.tsx @@ -81,7 +81,6 @@ export const NavFlyout: React.FunctionComponent = () => { preventDefault flyout={curFlyout} id="nav-flyout-default-link-3" - to="#nav-flyout-default-link-3" itemId="nav-flyout-default-link-3" isActive={activeItem === 'nav-flyout-default-link-3'} > diff --git a/packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx b/packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx index 4bebbcf090d..16810395f0c 100644 --- a/packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx +++ b/packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx @@ -384,13 +384,7 @@ export class NavDemo extends Component { Link 2 - + Link 3 From 06d7d62969336113bc10481e700148330ebe58e9 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Mon, 24 Oct 2022 17:09:14 -0400 Subject: [PATCH 03/12] fix(NavItem): remove aria-expanded prop --- packages/react-core/src/components/Nav/NavItem.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/packages/react-core/src/components/Nav/NavItem.tsx index 8107d1db666..66613e93ddd 100644 --- a/packages/react-core/src/components/Nav/NavItem.tsx +++ b/packages/react-core/src/components/Nav/NavItem.tsx @@ -170,8 +170,7 @@ export const NavItem: React.FunctionComponent = ({ ); const ariaFlyoutProps = { - 'aria-haspopup': 'menu', - 'aria-expanded': flyoutVisible + 'aria-haspopup': 'menu' }; const renderDefaultLink = (context: any): React.ReactNode => { From 435d2aeb5cc0e7d374b9ddde788b12ce472e4bc8 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Thu, 10 Nov 2022 10:55:24 -0500 Subject: [PATCH 04/12] fix(NavItem): use a button for Component if there is a flyout --- packages/react-core/src/components/Nav/NavItem.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/packages/react-core/src/components/Nav/NavItem.tsx index 66613e93ddd..ea59a9a2d85 100644 --- a/packages/react-core/src/components/Nav/NavItem.tsx +++ b/packages/react-core/src/components/Nav/NavItem.tsx @@ -77,8 +77,8 @@ export const NavItem: React.FunctionComponent = ({ const ref = React.useRef(); const flyoutVisible = ref === flyoutRef; const popperRef = React.useRef(); - const Component = component as any; const hasFlyout = flyout !== undefined; + const Component = hasFlyout ? 'button' : (component as any); const showFlyout = (show: boolean, override?: boolean) => { if ((!flyoutVisible || override) && show) { @@ -170,9 +170,12 @@ export const NavItem: React.FunctionComponent = ({ ); const ariaFlyoutProps = { - 'aria-haspopup': 'menu' + 'aria-haspopup': 'menu', + 'aria-expanded': flyoutVisible }; + const tabIndex = isNavOpen ? null : -1; + const renderDefaultLink = (context: any): React.ReactNode => { const preventLinkDefault = preventDefault || !to; return ( @@ -186,7 +189,7 @@ export const NavItem: React.FunctionComponent = ({ className )} aria-current={isActive ? 'page' : null} - tabIndex={isNavOpen ? null : '-1'} + tabIndex={tabIndex} {...(hasFlyout && { ...ariaFlyoutProps })} {...props} > @@ -203,7 +206,7 @@ export const NavItem: React.FunctionComponent = ({ ...(styleChildren && { className: css(styles.navLink, isActive && styles.modifiers.current, child.props && child.props.className) }), - tabIndex: child.props.tabIndex || isNavOpen ? null : -1, + tabIndex: child.props.tabIndex || tabIndex, children: hasFlyout ? ( {child.props.children} From 1699a7cc3e20c949ece1b42d5ad67d263d42db00 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Thu, 24 Nov 2022 19:17:22 -0500 Subject: [PATCH 05/12] fix(NavItem): allow flyout to be opened on pressing Enter --- packages/react-core/src/components/Nav/NavItem.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/packages/react-core/src/components/Nav/NavItem.tsx index d455ca81fc1..442b8806ecb 100644 --- a/packages/react-core/src/components/Nav/NavItem.tsx +++ b/packages/react-core/src/components/Nav/NavItem.tsx @@ -122,7 +122,7 @@ export const NavItem: React.FunctionComponent = ({ return; } - if (key === ' ' || key === 'ArrowRight') { + if (key === ' ' || key === 'Enter' || key === 'ArrowRight') { event.stopPropagation(); event.preventDefault(); if (!flyoutVisible) { From af8d64f2e2324b1f0c646099ee26f8efcbc679be Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Mon, 28 Nov 2022 14:16:25 -0500 Subject: [PATCH 06/12] fix(MenuItem): disallow flyoutMenu and link props to both be defined --- .../src/components/Menu/MenuItem.tsx | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/react-core/src/components/Menu/MenuItem.tsx b/packages/react-core/src/components/Menu/MenuItem.tsx index 1af964b3850..ce845c2e362 100644 --- a/packages/react-core/src/components/Menu/MenuItem.tsx +++ b/packages/react-core/src/components/Menu/MenuItem.tsx @@ -15,15 +15,13 @@ import { canUseDOM } from '../../helpers/util'; import { useIsomorphicLayoutEffect } from '../../helpers/useIsomorphicLayout'; import { GenerateId } from '../../helpers/GenerateId/GenerateId'; -export interface MenuItemProps extends Omit, 'onClick'> { +interface CommonMenuItemProps extends Omit, 'onClick'> { /** Content rendered inside the menu list item. */ children?: React.ReactNode; /** Additional classes added to the menu list item */ className?: string; /** Identifies the component in the Menu onSelect or onActionClick callback */ itemId?: any; - /** Target navigation link */ - to?: string; /** @beta Flag indicating the item has a checkbox */ hasCheck?: boolean; /** Flag indicating whether the item is active */ @@ -54,8 +52,6 @@ export interface MenuItemProps extends Omit, 'onC isFocused?: boolean; /** Flag indicating the item is in danger state */ isDanger?: boolean; - /** @beta Flyout menu */ - flyoutMenu?: React.ReactElement; /** @beta Callback function when mouse leaves trigger */ onShowFlyout?: (event?: any) => void; /** @beta Drilldown menu of the item. Should be a Menu or DrilldownMenu type. */ @@ -70,6 +66,22 @@ export interface MenuItemProps extends Omit, 'onC innerRef?: React.Ref; } +type ConditionalMenuItemProps = + | { + /** Target navigation link */ + to?: string; + /** @beta Flyout menu. Disallowed if nav link is defined */ + flyoutMenu?: never; + } + | { + /** Target navigation link. Disallowed if flyoutMenu is defined */ + to?: never; + /** @beta Flyout menu */ + flyoutMenu?: React.ReactElement; + }; + +export type MenuItemProps = CommonMenuItemProps & ConditionalMenuItemProps; + const FlyoutContext = React.createContext({ direction: 'right' as 'left' | 'right' }); From 5ec7ea6b8dd0741e090837df03523a7e7d16d7f6 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Mon, 28 Nov 2022 15:36:48 -0500 Subject: [PATCH 07/12] docs(Navigation): remove links from MenuItems with flyouts --- .../src/components/Nav/examples/NavFlyout.tsx | 14 ++------------ packages/react-core/src/demos/Nav.md | 4 ++-- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/packages/react-core/src/components/Nav/examples/NavFlyout.tsx b/packages/react-core/src/components/Nav/examples/NavFlyout.tsx index 59ae5d5e78f..be4ae9f12e3 100644 --- a/packages/react-core/src/components/Nav/examples/NavFlyout.tsx +++ b/packages/react-core/src/components/Nav/examples/NavFlyout.tsx @@ -19,12 +19,7 @@ export const NavFlyout: React.FunctionComponent = () => { - + Next menu {Array.apply(0, Array(numFlyouts - depth)).map((_item, index: number) => ( @@ -38,12 +33,7 @@ export const NavFlyout: React.FunctionComponent = () => { Menu {depth} item {index} ))} - + Next menu diff --git a/packages/react-core/src/demos/Nav.md b/packages/react-core/src/demos/Nav.md index c2ef14b11a7..0e4106f6781 100644 --- a/packages/react-core/src/demos/Nav.md +++ b/packages/react-core/src/demos/Nav.md @@ -1463,7 +1463,7 @@ class VerticalPage extends React.Component { - + Additional settings {[...Array(numFlyouts - depth).keys()].map(j => ( @@ -1471,7 +1471,7 @@ class VerticalPage extends React.Component { Settings menu {depth} item {j} ))} - + Additional settings From de73623518454a560d62f268e789cace5d6d1b28 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Wed, 30 Nov 2022 12:12:08 -0500 Subject: [PATCH 08/12] fix(SelectOption): make props conditional to align with MenuItemProps --- .../next/components/Select/SelectOption.tsx | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/react-core/src/next/components/Select/SelectOption.tsx b/packages/react-core/src/next/components/Select/SelectOption.tsx index d731bc59ed4..fdabbb577b3 100644 --- a/packages/react-core/src/next/components/Select/SelectOption.tsx +++ b/packages/react-core/src/next/components/Select/SelectOption.tsx @@ -2,7 +2,7 @@ import React from 'react'; import { css } from '@patternfly/react-styles'; import { MenuItemProps, MenuItem } from '../../../components/Menu'; -export interface SelectOptionProps extends Omit { +interface CommonSelectOptionProps extends Omit { /** Anything which can be rendered in a select option */ children?: React.ReactNode; /** Classes applied to root element of select option */ @@ -19,6 +19,22 @@ export interface SelectOptionProps extends Omit { isFocused?: boolean; } +type ConditionalSelectOptionProps = + | { + /** Target navigation link */ + to?: string; + /** Flyout menu. Disallowed if nav link is defined */ + flyoutMenu?: never; + } + | { + /** Target navigation link. Disallowed if flyoutMenu is defined */ + to?: never; + /** Flyout menu */ + flyoutMenu?: React.ReactElement; + }; + +export type SelectOptionProps = CommonSelectOptionProps & ConditionalSelectOptionProps; + export const SelectOption: React.FunctionComponent = ({ children, className, From c8cdd3ad95a6225bfb0edd31c0060b357d5fd2eb Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Wed, 30 Nov 2022 12:12:24 -0500 Subject: [PATCH 09/12] fix(DropdownItem): make props conditional to align with MenuItemProps --- .../next/components/Dropdown/DropdownItem.tsx | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx b/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx index e95335bb39e..22238577ee5 100644 --- a/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx +++ b/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx @@ -3,7 +3,7 @@ import { css } from '@patternfly/react-styles'; import { MenuItemProps, MenuItem } from '../../../components/Menu'; import { useOUIAProps, OUIAProps } from '../../../helpers'; -export interface DropdownItemProps extends Omit, OUIAProps { +interface CommonDropdownItemProps extends Omit, OUIAProps { /** Anything which can be rendered in a dropdown item */ children?: React.ReactNode; /** Classes applied to root element of dropdown item */ @@ -16,6 +16,22 @@ export interface DropdownItemProps extends Omit, OUIAProps ouiaSafe?: boolean; } +type ConditionalDropdownItemProps = + | { + /** Target navigation link */ + to?: string; + /** Flyout menu. Disallowed if nav link is defined */ + flyoutMenu?: never; + } + | { + /** Target navigation link. Disallowed if flyoutMenu is defined */ + to?: never; + /** Flyout menu */ + flyoutMenu?: React.ReactElement; + }; + +export type DropdownItemProps = CommonDropdownItemProps & ConditionalDropdownItemProps; + export const DropdownItem: React.FunctionComponent = ({ children, className, From 5abd5691b2adab6fa440583091ac3af7fc539020 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Thu, 8 Dec 2022 11:00:40 -0500 Subject: [PATCH 10/12] fix(NavItem): revert conditional prop changes --- .../src/components/Menu/MenuItem.tsx | 22 ++++-------------- .../react-core/src/components/Nav/NavItem.tsx | 22 ++++-------------- .../next/components/Dropdown/DropdownItem.tsx | 23 ++++--------------- .../next/components/Select/SelectOption.tsx | 18 +-------------- 4 files changed, 16 insertions(+), 69 deletions(-) diff --git a/packages/react-core/src/components/Menu/MenuItem.tsx b/packages/react-core/src/components/Menu/MenuItem.tsx index ce845c2e362..1af964b3850 100644 --- a/packages/react-core/src/components/Menu/MenuItem.tsx +++ b/packages/react-core/src/components/Menu/MenuItem.tsx @@ -15,13 +15,15 @@ import { canUseDOM } from '../../helpers/util'; import { useIsomorphicLayoutEffect } from '../../helpers/useIsomorphicLayout'; import { GenerateId } from '../../helpers/GenerateId/GenerateId'; -interface CommonMenuItemProps extends Omit, 'onClick'> { +export interface MenuItemProps extends Omit, 'onClick'> { /** Content rendered inside the menu list item. */ children?: React.ReactNode; /** Additional classes added to the menu list item */ className?: string; /** Identifies the component in the Menu onSelect or onActionClick callback */ itemId?: any; + /** Target navigation link */ + to?: string; /** @beta Flag indicating the item has a checkbox */ hasCheck?: boolean; /** Flag indicating whether the item is active */ @@ -52,6 +54,8 @@ interface CommonMenuItemProps extends Omit, 'onCl isFocused?: boolean; /** Flag indicating the item is in danger state */ isDanger?: boolean; + /** @beta Flyout menu */ + flyoutMenu?: React.ReactElement; /** @beta Callback function when mouse leaves trigger */ onShowFlyout?: (event?: any) => void; /** @beta Drilldown menu of the item. Should be a Menu or DrilldownMenu type. */ @@ -66,22 +70,6 @@ interface CommonMenuItemProps extends Omit, 'onCl innerRef?: React.Ref; } -type ConditionalMenuItemProps = - | { - /** Target navigation link */ - to?: string; - /** @beta Flyout menu. Disallowed if nav link is defined */ - flyoutMenu?: never; - } - | { - /** Target navigation link. Disallowed if flyoutMenu is defined */ - to?: never; - /** @beta Flyout menu */ - flyoutMenu?: React.ReactElement; - }; - -export type MenuItemProps = CommonMenuItemProps & ConditionalMenuItemProps; - const FlyoutContext = React.createContext({ direction: 'right' as 'left' | 'right' }); diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/packages/react-core/src/components/Nav/NavItem.tsx index 442b8806ecb..18428eada01 100644 --- a/packages/react-core/src/components/Nav/NavItem.tsx +++ b/packages/react-core/src/components/Nav/NavItem.tsx @@ -7,13 +7,15 @@ import { useOUIAProps, OUIAProps } from '../../helpers'; import { Popper } from '../../helpers/Popper/Popper'; import AngleRightIcon from '@patternfly/react-icons/dist/esm/icons/angle-right-icon'; -interface CommonNavItemProps extends Omit, 'onClick'>, OUIAProps { +export interface NavItemProps extends Omit, 'onClick'>, OUIAProps { /** Content rendered inside the nav item. */ children?: React.ReactNode; /** Whether to set className on children when React.isValidElement(children) */ styleChildren?: boolean; /** Additional classes added to the nav item */ className?: string; + /** Target navigation link */ + to?: string; /** Flag indicating whether the item is active */ isActive?: boolean; /** Group identifier, will be returned with the onToggle and onSelect callback passed to the Nav component */ @@ -26,6 +28,8 @@ interface CommonNavItemProps extends Omit, 'o onClick?: NavSelectClickHandler; /** Component used to render NavItems if React.isValidElement(children) is false */ component?: React.ReactNode; + /** Flyout of a nav item. This should be a Menu component. */ + flyout?: React.ReactElement; /** Callback when flyout is opened or closed */ onShowFlyout?: () => void; /** @beta Opt-in for updated popper that does not use findDOMNode. */ @@ -38,22 +42,6 @@ interface CommonNavItemProps extends Omit, 'o ouiaSafe?: boolean; } -type ConditionalNavItemProps = - | { - /** Target navigation link */ - to?: string; - /** Flyout of a nav item. Disallowed if nav link is defined */ - flyout?: never; - } - | { - /** Target navigation link. Disallowed if flyout is defined */ - to?: never; - /** Flyout of a nav item. This should be a Menu component. */ - flyout?: React.ReactNode; - }; - -export type NavItemProps = CommonNavItemProps & ConditionalNavItemProps; - export const NavItem: React.FunctionComponent = ({ children, styleChildren = true, diff --git a/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx b/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx index 22238577ee5..100a6290034 100644 --- a/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx +++ b/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx @@ -3,46 +3,33 @@ import { css } from '@patternfly/react-styles'; import { MenuItemProps, MenuItem } from '../../../components/Menu'; import { useOUIAProps, OUIAProps } from '../../../helpers'; -interface CommonDropdownItemProps extends Omit, OUIAProps { +export interface DropdownItemProps extends Omit, OUIAProps { /** Anything which can be rendered in a dropdown item */ children?: React.ReactNode; /** Classes applied to root element of dropdown item */ className?: string; /** Description of the dropdown item */ description?: React.ReactNode; + /** Identifies the component in the dropdown onSelect callback */ + itemId?: any; /** Value to overwrite the randomly generated data-ouia-component-id.*/ ouiaId?: number | string; /** Set the value of data-ouia-safe. Only set to true when the component is in a static state, i.e. no animations are occurring. At all other times, this value must be false. */ ouiaSafe?: boolean; } -type ConditionalDropdownItemProps = - | { - /** Target navigation link */ - to?: string; - /** Flyout menu. Disallowed if nav link is defined */ - flyoutMenu?: never; - } - | { - /** Target navigation link. Disallowed if flyoutMenu is defined */ - to?: never; - /** Flyout menu */ - flyoutMenu?: React.ReactElement; - }; - -export type DropdownItemProps = CommonDropdownItemProps & ConditionalDropdownItemProps; - export const DropdownItem: React.FunctionComponent = ({ children, className, description, + itemId, ouiaId, ouiaSafe, ...props }: DropdownItemProps) => { const ouiaProps = useOUIAProps(DropdownItem.displayName, ouiaId, ouiaSafe); return ( - + {children} ); diff --git a/packages/react-core/src/next/components/Select/SelectOption.tsx b/packages/react-core/src/next/components/Select/SelectOption.tsx index fdabbb577b3..d731bc59ed4 100644 --- a/packages/react-core/src/next/components/Select/SelectOption.tsx +++ b/packages/react-core/src/next/components/Select/SelectOption.tsx @@ -2,7 +2,7 @@ import React from 'react'; import { css } from '@patternfly/react-styles'; import { MenuItemProps, MenuItem } from '../../../components/Menu'; -interface CommonSelectOptionProps extends Omit { +export interface SelectOptionProps extends Omit { /** Anything which can be rendered in a select option */ children?: React.ReactNode; /** Classes applied to root element of select option */ @@ -19,22 +19,6 @@ interface CommonSelectOptionProps extends Omit { isFocused?: boolean; } -type ConditionalSelectOptionProps = - | { - /** Target navigation link */ - to?: string; - /** Flyout menu. Disallowed if nav link is defined */ - flyoutMenu?: never; - } - | { - /** Target navigation link. Disallowed if flyoutMenu is defined */ - to?: never; - /** Flyout menu */ - flyoutMenu?: React.ReactElement; - }; - -export type SelectOptionProps = CommonSelectOptionProps & ConditionalSelectOptionProps; - export const SelectOption: React.FunctionComponent = ({ children, className, From dbcd85b08cf1d07748e19c91b4d81688b824dbf0 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Thu, 8 Dec 2022 11:05:37 -0500 Subject: [PATCH 11/12] fix(NavItem): fix opening nested MenuItem-based flyouts with keyboard --- packages/react-core/src/components/Menu/MenuItem.tsx | 1 + packages/react-core/src/components/Nav/NavItem.tsx | 10 ++++------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/react-core/src/components/Menu/MenuItem.tsx b/packages/react-core/src/components/Menu/MenuItem.tsx index 1af964b3850..86f44f1a436 100644 --- a/packages/react-core/src/components/Menu/MenuItem.tsx +++ b/packages/react-core/src/components/Menu/MenuItem.tsx @@ -202,6 +202,7 @@ const MenuItemBase: React.FunctionComponent = ({ if (key === ' ' || key === 'Enter' || key === 'ArrowRight' || type === 'click') { event.stopPropagation(); + event.preventDefault(); if (!flyoutVisible) { showFlyout(true); setFlyoutTarget(target as HTMLElement); diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/packages/react-core/src/components/Nav/NavItem.tsx index 18428eada01..a1e0c1552c5 100644 --- a/packages/react-core/src/components/Nav/NavItem.tsx +++ b/packages/react-core/src/components/Nav/NavItem.tsx @@ -106,11 +106,7 @@ export const NavItem: React.FunctionComponent = ({ const key = event.key; const target = event.target as HTMLElement; - if (!(popperRef?.current?.contains(target) || (hasFlyout && ref?.current?.contains(target)))) { - return; - } - - if (key === ' ' || key === 'Enter' || key === 'ArrowRight') { + if ((key === ' ' || key === 'Enter' || key === 'ArrowRight') && hasFlyout && ref?.current?.contains(target)) { event.stopPropagation(); event.preventDefault(); if (!flyoutVisible) { @@ -119,7 +115,9 @@ export const NavItem: React.FunctionComponent = ({ } } - if (key === 'Escape' || key === 'ArrowLeft') { + // We only want the NavItem to handle closing a flyout menu if only the first level flyout is open. + // Otherwise, MenuItem should handle closing its flyouts + if ((key === 'Escape' || key === 'ArrowLeft') && popperRef?.current?.querySelectorAll('.pf-c-menu').length === 1) { if (flyoutVisible) { event.stopPropagation(); event.preventDefault(); From 3db3fa14a45e04d8f080cb7554c74968773199b6 Mon Sep 17 00:00:00 2001 From: Brian Rui Date: Thu, 8 Dec 2022 11:50:42 -0500 Subject: [PATCH 12/12] fix(NavItem): add a console.err message if to and flyout are used together --- packages/react-core/src/components/Menu/MenuItem.tsx | 4 ++-- packages/react-core/src/components/Nav/NavItem.tsx | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/react-core/src/components/Menu/MenuItem.tsx b/packages/react-core/src/components/Menu/MenuItem.tsx index 86f44f1a436..484ab70a90a 100644 --- a/packages/react-core/src/components/Menu/MenuItem.tsx +++ b/packages/react-core/src/components/Menu/MenuItem.tsx @@ -22,7 +22,7 @@ export interface MenuItemProps extends Omit, 'onC className?: string; /** Identifies the component in the Menu onSelect or onActionClick callback */ itemId?: any; - /** Target navigation link */ + /** Target navigation link. Should not be used if the flyout prop is defined. */ to?: string; /** @beta Flag indicating the item has a checkbox */ hasCheck?: boolean; @@ -54,7 +54,7 @@ export interface MenuItemProps extends Omit, 'onC isFocused?: boolean; /** Flag indicating the item is in danger state */ isDanger?: boolean; - /** @beta Flyout menu */ + /** @beta Flyout menu. Should not be used if the to prop is defined. */ flyoutMenu?: React.ReactElement; /** @beta Callback function when mouse leaves trigger */ onShowFlyout?: (event?: any) => void; diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/packages/react-core/src/components/Nav/NavItem.tsx index a1e0c1552c5..dfcacc6b1e9 100644 --- a/packages/react-core/src/components/Nav/NavItem.tsx +++ b/packages/react-core/src/components/Nav/NavItem.tsx @@ -14,7 +14,7 @@ export interface NavItemProps extends Omit, ' styleChildren?: boolean; /** Additional classes added to the nav item */ className?: string; - /** Target navigation link */ + /** Target navigation link. Should not be used if the flyout prop is defined. */ to?: string; /** Flag indicating whether the item is active */ isActive?: boolean; @@ -28,7 +28,7 @@ export interface NavItemProps extends Omit, ' onClick?: NavSelectClickHandler; /** Component used to render NavItems if React.isValidElement(children) is false */ component?: React.ReactNode; - /** Flyout of a nav item. This should be a Menu component. */ + /** Flyout of a nav item. This should be a Menu component. Should not be used if the to prop is defined. */ flyout?: React.ReactElement; /** Callback when flyout is opened or closed */ onShowFlyout?: () => void; @@ -71,6 +71,12 @@ export const NavItem: React.FunctionComponent = ({ const hasFlyout = flyout !== undefined; const Component = hasFlyout ? 'button' : (component as any); + // A NavItem should not be both a link and a flyout + if (to && hasFlyout) { + // eslint-disable-next-line no-console + console.error('NavItem cannot have both "to" and "flyout" props.'); + } + const showFlyout = (show: boolean, override?: boolean) => { if ((!flyoutVisible || override) && show) { setFlyoutRef(ref);