refactor: return previous selection state on link item click

Author: ze-flo

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(
           <TestMenu
             items={[
-              { value: 'link-1', href: '#1', isExternal: true },
-              { value: 'link-2', href: '#2', isExternal: true, disabled: true }
+              { value: 'link-1', href: '#1', external: true },
+              { value: 'link-2', href: '#2', external: true, disabled: true }
             ]}
           />
         );
         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(
           <TestMenu
             items={[
               { value: 'link-1', href: '#1' },
               { value: 'link-2', href: '#2' }
             ]}
+            onChange={onChangeSpy}
           />
         );
         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(
+          <TestMenu
+            items={[
+              { value: 'link-1', href: '#1' },
+              { value: 'link-2', href: '#2', selected: true }
+            ]}
+            onChange={onChangeSpy}
+          />
+        );
+        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(
           <TestMenu
             items={[
@@ -1317,7 +1366,7 @@ describe('MenuContainer', () => {
         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(
           <TestMenu
             items={[
@@ -1328,18 +1377,31 @@ describe('MenuContainer', () => {
           />
         );
         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');
       });
     });
   });

packages/menu/src/useMenu.ts

@@ -163,24 +163,15 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
 
       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;
-      } 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 = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
 
   const getSelectedItems = useCallback(
     ({ value, type, name, label, selected, href }: IMenuItemBase) => {
-      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 = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
       });
     },
     [
-      selectedItems,
       getSelectedItems,
       state.nestedPathIds,
       isExpandedControlled,