diff --git a/packages/react-core/src/components/Menu/MenuItem.tsx b/packages/react-core/src/components/Menu/MenuItem.tsx index 1af964b3850..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; @@ -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 31827f0ddf3..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; @@ -68,8 +68,14 @@ 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); + + // 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) { @@ -106,11 +112,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 === 'ArrowRight') { + if ((key === ' ' || key === 'Enter' || key === 'ArrowRight') && hasFlyout && ref?.current?.contains(target)) { event.stopPropagation(); event.preventDefault(); if (!flyoutVisible) { @@ -119,7 +121,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(); @@ -165,6 +169,8 @@ export const NavItem: React.FunctionComponent = ({ 'aria-expanded': flyoutVisible }; + const tabIndex = isNavOpen ? null : -1; + const renderDefaultLink = (context: any): React.ReactNode => { const preventLinkDefault = preventDefault || !to; return ( @@ -178,7 +184,7 @@ export const NavItem: React.FunctionComponent = ({ className )} aria-current={isActive ? 'page' : null} - tabIndex={isNavOpen ? null : '-1'} + tabIndex={tabIndex} {...(hasFlyout && { ...ariaFlyoutProps })} {...props} > @@ -195,7 +201,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} diff --git a/packages/react-core/src/components/Nav/examples/NavFlyout.tsx b/packages/react-core/src/components/Nav/examples/NavFlyout.tsx index 503b84fe9b0..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 @@ -81,7 +71,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-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 diff --git a/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx b/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx index e95335bb39e..100a6290034 100644 --- a/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx +++ b/packages/react-core/src/next/components/Dropdown/DropdownItem.tsx @@ -10,6 +10,8 @@ export interface DropdownItemProps extends Omit, OUIAProps 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. */ @@ -20,13 +22,14 @@ 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-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