From a5c22a9973ec8cc84fe3b403feee9a0b01137f78 Mon Sep 17 00:00:00 2001 From: Florent Mathieu Date: Wed, 23 Apr 2025 13:24:17 -1000 Subject: [PATCH 1/8] fix(menu): correctly identifies link menuitem has the same URL as the page --- packages/menu/demo/stories/MenuStory.tsx | 18 ++++++- packages/menu/src/MenuContainer.spec.tsx | 66 +++++++++++++++++++++--- packages/menu/src/useMenu.ts | 45 ++++++++++------ 3 files changed, 105 insertions(+), 24 deletions(-) diff --git a/packages/menu/demo/stories/MenuStory.tsx b/packages/menu/demo/stories/MenuStory.tsx index a60b8ecf8..a7898724e 100644 --- a/packages/menu/demo/stories/MenuStory.tsx +++ b/packages/menu/demo/stories/MenuStory.tsx @@ -33,6 +33,15 @@ type MenuItemProps = { isSelected?: boolean; }; +/** + * [1] For a disabled link to be valid, it must have: + * - `aria-disabled="true"` + * - `role="link"`, or `role="menuitem"` if within a menu + * - an `undefined` `href` + * + * @example Learn something! + * @see https://www.scottohara.me/blog/2021/05/28/disabled-links.html + */ const Item = ({ item, getItemProps, focusedValue, isSelected }: MenuItemProps) => { const itemProps = getItemProps({ item }); @@ -60,7 +69,14 @@ const Item = ({ item, getItemProps, focusedValue, isSelected }: MenuItemProps) = {itemProps.href ? ( )} - className="w-full rounded-sm outline-offset-0 transition-none border-width-none" + href={item.disabled ? undefined : itemProps.href} + 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 && ( diff --git a/packages/menu/src/MenuContainer.spec.tsx b/packages/menu/src/MenuContainer.spec.tsx index 427f195ba..69b5e89b0 100644 --- a/packages/menu/src/MenuContainer.spec.tsx +++ b/packages/menu/src/MenuContainer.spec.tsx @@ -283,14 +283,44 @@ 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 external anchor attributes, only if not disabled', () => { + const { getByTestId } = render( + + ); + const menu = getByTestId('menu'); + + expect(menu.firstChild).toHaveAttribute('target', '_blank'); + expect(menu.firstChild).toHaveAttribute('rel', 'noopener noreferrer'); + + expect(menu.childNodes[1]).not.toHaveAttribute('target', '_blank'); + expect(menu.childNodes[1]).not.toHaveAttribute('rel', 'noopener noreferrer'); + }); + + it('applies the correct aria-current attribute to active link', async () => { + const { getByTestId, getByText } = render( + + ); + const trigger = getByTestId('trigger'); + const link = getByText('link-2'); - expect(menu.firstChild).toHaveAttribute('target', '_blank'); - expect(menu.firstChild).toHaveAttribute('rel', 'noopener noreferrer'); + await waitFor(async () => { + await user.click(trigger); + await user.click(link); + }); + + expect(link).toHaveAttribute('aria-current', 'page'); + }); }); describe('focus', () => { @@ -1265,6 +1295,28 @@ describe('MenuContainer', () => { expect(changeTypes).toContain(StateChangeTypes.MenuItemKeyDownPrevious); }); }); + + describe('navigational menu items (links)', () => { + it('applies the correct aria-current attribute to 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'); + }); + }); }); describe('error handling', () => { diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts index c52dad233..e4acb7066 100644 --- a/packages/menu/src/useMenu.ts +++ b/packages/menu/src/useMenu.ts @@ -155,7 +155,7 @@ export const useMenu = { + (value: string, type?: string, name?: string, href?: string): boolean | undefined => { let isSelected; if (type === 'checkbox') { @@ -164,6 +164,10 @@ export const useMenu = item.name === name)[0]; isSelected = match?.value === value; + } else if (href) { + const current = controlledSelectedItems[0]; + + isSelected = current?.value === value; } return isSelected; @@ -218,10 +222,10 @@ export const useMenu = { + ({ value, type, name, label, selected, href }: IMenuItemBase) => { let changes: ISelectedItem[] | null = [...controlledSelectedItems]; - if (!type) return null; + if (!(type || href)) return null; const selectedItem = { value, @@ -236,7 +240,7 @@ export const useMenu = item.name === name); if (index > -1) { @@ -815,7 +819,7 @@ export const useMenu = Date: Thu, 24 Apr 2025 21:03:14 -1000 Subject: [PATCH 2/8] feat(menu): prevent default when clicking a selected link --- packages/menu/src/MenuContainer.spec.tsx | 27 +++++++++++++++++++++++- packages/menu/src/useMenu.ts | 10 +++++---- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/packages/menu/src/MenuContainer.spec.tsx b/packages/menu/src/MenuContainer.spec.tsx index 69b5e89b0..7850aeb06 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 './'; @@ -1316,6 +1316,31 @@ describe('MenuContainer', () => { expect(link).toHaveAttribute('aria-current', 'page'); }); + + it('prevents default when clicking a selected link', async () => { + const { getByTestId, getByText } = render( + + ); + const trigger = getByTestId('trigger'); + const link = getByText('link-2'); + + await waitFor(async () => { + await user.click(trigger); + }); + + const event = createEvent.click(link); + event.preventDefault = jest.fn(); + fireEvent(link, event); + + expect(event.preventDefault).toHaveBeenCalledTimes(1); + expect(link).toHaveAttribute('aria-current', 'page'); + }); }); }); diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts index e4acb7066..230a77d50 100644 --- a/packages/menu/src/useMenu.ts +++ b/packages/menu/src/useMenu.ts @@ -432,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); @@ -872,8 +874,8 @@ 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 }) From a2da6eddb64b5d23e90b759f84b7445212f42d85 Mon Sep 17 00:00:00 2001 From: Florent Mathieu Date: Mon, 28 Apr 2025 15:28:30 -1000 Subject: [PATCH 3/8] refactor: do not update selection state for link items --- packages/menu/src/useMenu.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts index 230a77d50..8dd30463f 100644 --- a/packages/menu/src/useMenu.ts +++ b/packages/menu/src/useMenu.ts @@ -225,7 +225,7 @@ export const useMenu = { let changes: ISelectedItem[] | null = [...controlledSelectedItems]; - if (!(type || href)) return null; + if (!type || href) return null; const selectedItem = { value, @@ -240,7 +240,7 @@ export const useMenu = item.name === name); if (index > -1) { From 82c5cbd757a44f40acd930dbf236babf9cad755d Mon Sep 17 00:00:00 2001 From: Florent Mathieu Date: Mon, 28 Apr 2025 15:34:31 -1000 Subject: [PATCH 4/8] refactor: account for initial selected link items for selection --- packages/menu/src/useMenu.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts index 8dd30463f..ccf79864a 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]); @@ -165,14 +168,19 @@ export const useMenu = item.name === name)[0]; isSelected = current?.value === value; } return isSelected; }, - [controlledSelectedItems] + [controlledSelectedItems, initialSelectedItems, selectedItems] ); const getNextFocusedValue = useCallback( @@ -472,6 +480,7 @@ export const useMenu = Date: Mon, 28 Apr 2025 15:35:42 -1000 Subject: [PATCH 5/8] refactor: set href to undefined if link item is disabled --- packages/menu/src/useMenu.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts index ccf79864a..2103c043e 100644 --- a/packages/menu/src/useMenu.ts +++ b/packages/menu/src/useMenu.ts @@ -817,7 +817,7 @@ export const useMenu = Date: Mon, 28 Apr 2025 15:36:17 -1000 Subject: [PATCH 6/8] docs: document new props --- packages/menu/src/types.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 From 0a234c99ac87606831bec49c9cbf657a5b121107 Mon Sep 17 00:00:00 2001 From: Florent Mathieu Date: Mon, 28 Apr 2025 15:36:59 -1000 Subject: [PATCH 7/8] docs: update story --- packages/menu/demo/stories/MenuStory.tsx | 18 ++++-------------- packages/menu/demo/stories/data.ts | 11 +++++++++-- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/packages/menu/demo/stories/MenuStory.tsx b/packages/menu/demo/stories/MenuStory.tsx index a7898724e..205cb6b42 100644 --- a/packages/menu/demo/stories/MenuStory.tsx +++ b/packages/menu/demo/stories/MenuStory.tsx @@ -33,15 +33,6 @@ type MenuItemProps = { isSelected?: boolean; }; -/** - * [1] For a disabled link to be valid, it must have: - * - `aria-disabled="true"` - * - `role="link"`, or `role="menuitem"` if within a menu - * - an `undefined` `href` - * - * @example Learn something! - * @see https://www.scottohara.me/blog/2021/05/28/disabled-links.html - */ const Item = ({ item, getItemProps, focusedValue, isSelected }: MenuItemProps) => { const itemProps = getItemProps({ item }); @@ -63,13 +54,12 @@ 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 ? ( )} - href={item.disabled ? undefined : itemProps.href} className={classNames( ' w-full rounded-sm outline-offset-0 transition-none border-width-none', { @@ -79,7 +69,7 @@ const Item = ({ item, getItemProps, focusedValue, isSelected }: MenuItemProps) = )} > {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: [ From 56e51795f803f40d499e3f4388daf4d33ddfb1a2 Mon Sep 17 00:00:00 2001 From: Florent Mathieu Date: Tue, 29 Apr 2025 08:32:44 -1000 Subject: [PATCH 8/8] refactor: return previous selection state on link item click --- packages/menu/src/MenuContainer.spec.tsx | 88 ++++++++++++++++++++---- packages/menu/src/useMenu.ts | 19 ++--- 2 files changed, 80 insertions(+), 27 deletions(-) diff --git a/packages/menu/src/MenuContainer.spec.tsx b/packages/menu/src/MenuContainer.spec.tsx index 7850aeb06..b69d0fc49 100644 --- a/packages/menu/src/MenuContainer.spec.tsx +++ b/packages/menu/src/MenuContainer.spec.tsx @@ -284,31 +284,36 @@ describe('MenuContainer', () => { }); describe('navigational menu items (links)', () => { - it('applies external anchor attributes, only if not disabled', () => { + 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('applies the correct aria-current attribute to active link', async () => { + 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'); @@ -319,7 +324,51 @@ describe('MenuContainer', () => { await user.click(link); }); - expect(link).toHaveAttribute('aria-current', 'page'); + 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'); + + 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'); }); }); @@ -1297,7 +1346,7 @@ describe('MenuContainer', () => { }); describe('navigational menu items (links)', () => { - it('applies the correct aria-current attribute to selected link', async () => { + it('applies the correct aria-current attribute to user-selected link', async () => { const { getByTestId, getByText } = render( { expect(link).toHaveAttribute('aria-current', 'page'); }); - it('prevents default when clicking a selected link', async () => { + it('prevents default only when clicking on user-selected link', async () => { const { getByTestId, getByText } = render( { /> ); const trigger = getByTestId('trigger'); - const link = getByText('link-2'); + const firstLink = getByText('link-1'); + const secondLink = getByText('link-2'); await waitFor(async () => { await user.click(trigger); + await user.click(firstLink); }); - const event = createEvent.click(link); - event.preventDefault = jest.fn(); - fireEvent(link, event); + expect(firstLink).not.toHaveAttribute('aria-current'); - expect(event.preventDefault).toHaveBeenCalledTimes(1); - expect(link).toHaveAttribute('aria-current', 'page'); + 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'); }); }); }); diff --git a/packages/menu/src/useMenu.ts b/packages/menu/src/useMenu.ts index 2103c043e..013a05e55 100644 --- a/packages/menu/src/useMenu.ts +++ b/packages/menu/src/useMenu.ts @@ -163,24 +163,15 @@ export const useMenu = 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; - } else if (href) { - const selection = - Array.isArray(selectedItems) && selectedItems.length - ? selectedItems - : initialSelectedItems; - - const current = selection.filter(item => item.name === name)[0]; - - isSelected = current?.value === value; } return isSelected; }, - [controlledSelectedItems, initialSelectedItems, selectedItems] + [controlledSelectedItems] ); const getNextFocusedValue = useCallback( @@ -231,9 +222,10 @@ export const useMenu = { - let changes: ISelectedItem[] | null = [...controlledSelectedItems]; + if (href) return controlledSelectedItems; + if (!type) return null; - if (!type || href) return null; + let changes: ISelectedItem[] | null = [...controlledSelectedItems]; const selectedItem = { value, @@ -480,7 +472,6 @@ export const useMenu =