diff --git a/packages/menu/demo/stories/MenuStory.tsx b/packages/menu/demo/stories/MenuStory.tsx index a60b8ecf8..205cb6b42 100644 --- a/packages/menu/demo/stories/MenuStory.tsx +++ b/packages/menu/demo/stories/MenuStory.tsx @@ -54,16 +54,22 @@ const Item = ({ item, getItemProps, focusedValue, isSelected }: MenuItemProps) = 'cursor-pointer': !item.disabled, 'cursor-default': item.disabled })} - role={itemProps.href ? 'none' : undefined} - {...(!itemProps.href && (itemProps as LiHTMLAttributes))} + role={item.href ? 'none' : undefined} + {...(!item.href && (itemProps as LiHTMLAttributes))} > - {itemProps.href ? ( + {item.href ? ( )} - className="w-full rounded-sm outline-offset-0 transition-none border-width-none" + className={classNames( + ' w-full rounded-sm outline-offset-0 transition-none border-width-none', + { + 'text-grey-400': item.disabled, + 'cursor-default': item.disabled + } + )} > {itemChildren} - {!!item.isExternal && ( + {!!item.external && ( <> (opens in new window) diff --git a/packages/menu/demo/stories/data.ts b/packages/menu/demo/stories/data.ts index deb6bce22..8e14c7e4a 100644 --- a/packages/menu/demo/stories/data.ts +++ b/packages/menu/demo/stories/data.ts @@ -16,9 +16,16 @@ export const ITEMS: MenuItem[] = [ value: 'plant-04', label: 'Aloe Vera', href: 'https://en.wikipedia.org/wiki/Aloe_vera', - isExternal: false + external: false }, - { value: 'plant-05', label: 'Succulent' }, + { + value: 'plant-05', + label: 'Palm tree', + href: 'https://en.wikipedia.org/wiki/Palm_tree', + external: true, + disabled: true + }, + { value: 'plant-06', label: 'Succulent' }, { label: 'Choose favorites', items: [ diff --git a/packages/menu/src/MenuContainer.spec.tsx b/packages/menu/src/MenuContainer.spec.tsx index 427f195ba..b69d0fc49 100644 --- a/packages/menu/src/MenuContainer.spec.tsx +++ b/packages/menu/src/MenuContainer.spec.tsx @@ -6,7 +6,7 @@ */ import React, { useCallback, useRef, useState } from 'react'; -import { RenderResult, render, act, waitFor } from '@testing-library/react'; +import { act, createEvent, fireEvent, render, RenderResult, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { MenuItem, IMenuItemBase, IUseMenuProps, IUseMenuReturnValue } from './types'; import { MenuContainer } from './'; @@ -283,14 +283,93 @@ describe('MenuContainer', () => { expect(menu).not.toBeVisible(); }); - it('applies external anchor attributes', () => { - const { getByTestId } = render( - - ); - const menu = getByTestId('menu'); + describe('navigational menu items (links)', () => { + it('applies href and external anchor attributes, only when not disabled', () => { + const { getByTestId } = render( + + ); + const menu = getByTestId('menu'); + + expect(menu.firstChild).toHaveAttribute('href', '#1'); + expect(menu.firstChild).toHaveAttribute('target', '_blank'); + expect(menu.firstChild).toHaveAttribute('rel', 'noopener noreferrer'); + + expect(menu.childNodes[1]).not.toHaveAttribute('href'); + expect(menu.childNodes[1]).not.toHaveAttribute('target', '_blank'); + expect(menu.childNodes[1]).not.toHaveAttribute('rel', 'noopener noreferrer'); + }); + + it('tracks click events on links and alls the onChange function with the correct parameters', async () => { + const onChangeSpy = jest.fn(); + + const { getByTestId, getByText } = render( + + ); + const trigger = getByTestId('trigger'); + const link = getByText('link-2'); + + await waitFor(async () => { + await user.click(trigger); + await user.click(link); + }); + + expect(onChangeSpy.mock.calls[2][0]).toStrictEqual({ + type: 'menuItem:click', + value: 'link-2', + isExpanded: false, + selectedItems: [] + }); + + expect(link).not.toHaveAttribute('aria-current', 'page'); + }); + + it('supports setting the selected link via item.selected and ignore link selection changes', async () => { + const onChangeSpy = jest.fn(); + const { getByTestId, getByText } = render( + + ); + const trigger = getByTestId('trigger'); + const secondLink = getByText('link-2'); + + await waitFor(async () => { + await user.click(trigger); + }); + + expect(secondLink).toHaveAttribute('aria-current', 'page'); - expect(menu.firstChild).toHaveAttribute('target', '_blank'); - expect(menu.firstChild).toHaveAttribute('rel', 'noopener noreferrer'); + const firstLink = getByText('link-1'); + + await waitFor(async () => { + await user.click(trigger); + await user.click(firstLink); + }); + + expect(onChangeSpy.mock.calls[3][0]).toStrictEqual({ + type: 'menuItem:click', + value: 'link-1', + isExpanded: false, + selectedItems: [{ href: '#2', selected: true, value: 'link-2' }] + }); + + expect(firstLink).not.toHaveAttribute('aria-current'); + }); }); describe('focus', () => { @@ -1265,6 +1344,66 @@ describe('MenuContainer', () => { expect(changeTypes).toContain(StateChangeTypes.MenuItemKeyDownPrevious); }); }); + + describe('navigational menu items (links)', () => { + it('applies the correct aria-current attribute to user-selected link', async () => { + const { getByTestId, getByText } = render( + + ); + const trigger = getByTestId('trigger'); + const link = getByText('link-2'); + + await waitFor(async () => { + await user.click(trigger); + }); + + expect(link).toHaveAttribute('aria-current', 'page'); + }); + + it('prevents default only when clicking on user-selected link', async () => { + const { getByTestId, getByText } = render( + + ); + const trigger = getByTestId('trigger'); + const firstLink = getByText('link-1'); + const secondLink = getByText('link-2'); + + await waitFor(async () => { + await user.click(trigger); + await user.click(firstLink); + }); + + expect(firstLink).not.toHaveAttribute('aria-current'); + + const firstLinkClickEvent = createEvent.click(firstLink); + firstLinkClickEvent.preventDefault = jest.fn(); + fireEvent(firstLink, firstLinkClickEvent); + + // fire click event one more time to test behavior on previously clicked anchor + fireEvent(firstLink, firstLinkClickEvent); + + expect(firstLinkClickEvent.preventDefault).toHaveBeenCalledTimes(0); + + const secondLinkClickEvent = createEvent.click(secondLink); + secondLinkClickEvent.preventDefault = jest.fn(); + fireEvent(secondLink, secondLinkClickEvent); + + expect(secondLinkClickEvent.preventDefault).toHaveBeenCalledTimes(1); + expect(secondLink).toHaveAttribute('aria-current', 'page'); + }); + }); }); describe('error handling', () => { diff --git a/packages/menu/src/types.ts b/packages/menu/src/types.ts index d3826403d..20e5aa24b 100644 --- a/packages/menu/src/types.ts +++ b/packages/menu/src/types.ts @@ -14,7 +14,7 @@ export interface ISelectedItem { type?: 'radio' | 'checkbox'; disabled?: boolean; href?: string; - isExternal?: boolean; + external?: boolean; } export interface IMenuItemBase extends ISelectedItem { @@ -42,8 +42,10 @@ export interface IUseMenuProps { * @param {string} item.value Unique item value * @param {string} item.label Optional human-readable text value (defaults to `item.value`) * @param {string} item.name A shared name corresponding to an item radio group + * @param {string} item.href The URL to navigate to when the link item is clicked + * @param {boolean} item.external Indicates that link item is an external link * @param {boolean} item.disabled Indicates the item is not interactive - * @param {boolean} item.selected Sets initial selection for the option + * @param {boolean} item.selected Sets initial selection for the option. The initial selection persists for link items. * @param {boolean} item.isNext - Indicates the item transitions to a nested menu * @param {boolean} item.isPrevious - Indicates the item will transition back from a nested menu * @param {boolean} item.separator Indicates the item is a placeholder for a separator diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts index c52dad233..013a05e55 100644 --- a/packages/menu/src/useMenu.ts +++ b/packages/menu/src/useMenu.ts @@ -73,9 +73,12 @@ export const useMenu = - menuItems.filter( - item => !!(item.type && ['radio', 'checkbox'].includes(item.type) && item.selected) - ), + menuItems.filter(item => { + return !!( + (item.href || item.type === 'radio' || item.type === 'checkbox') && + item.selected + ); + }), [menuItems] ); const values = useMemo(() => menuItems.map(item => item.value), [menuItems]); @@ -155,12 +158,12 @@ export const useMenu = { + (value: string, type?: string, name?: string, href?: string): boolean | undefined => { let isSelected; if (type === 'checkbox') { isSelected = !!controlledSelectedItems.find(item => item.value === value); - } else if (type === 'radio') { + } else if (type === 'radio' || href) { const match = controlledSelectedItems.filter(item => item.name === name)[0]; isSelected = match?.value === value; @@ -218,11 +221,12 @@ export const useMenu = { - let changes: ISelectedItem[] | null = [...controlledSelectedItems]; - + ({ value, type, name, label, selected, href }: IMenuItemBase) => { + if (href) return controlledSelectedItems; if (!type) return null; + let changes: ISelectedItem[] | null = [...controlledSelectedItems]; + const selectedItem = { value, type, @@ -428,15 +432,17 @@ export const useMenu = { + (event: React.MouseEvent, item: IMenuItemBase) => { let changeType = StateChangeTypes.MenuItemClick; - const { isNext, isPrevious } = item; + const { isNext, isPrevious, href, selected } = item; const isTransitionItem = isNext || isPrevious; if (isNext) { changeType = StateChangeTypes.MenuItemClickNext; } else if (isPrevious) { changeType = StateChangeTypes.MenuItemClickPrevious; + } else if (href && selected) { + event.preventDefault(); } const nextSelection = getSelectedItems(item); @@ -802,7 +808,7 @@ export const useMenu = - handleItemClick({ ...item, label, selected, isNext, isPrevious }) + onClick: composeEventHandlers(onClick, (e: React.MouseEvent) => + handleItemClick(e, { ...item, label, selected, isNext, isPrevious }) ), onKeyDown: composeEventHandlers(onKeyDown, (e: React.KeyboardEvent) => handleItemKeyDown(e, { ...item, label, selected, isNext, isPrevious })