fix: missing li tag when href is specified (#4168)

* fix(items): items in list should wrapped in li in case of a * chore: adding the tests
nextui-org · Nov 29, 2024 · 0b5ceb9 · 0b5ceb9
1 parent 53c3338
commit 0b5ceb9
Show file tree

Hide file tree

Showing 10 changed files with 166 additions and 35 deletions.
diff --git a/.changeset/nasty-bees-sniff.md b/.changeset/nasty-bees-sniff.md
@@ -0,0 +1,7 @@
+---
+"@nextui-org/listbox": patch
+"@nextui-org/menu": patch
+"@nextui-org/pagination": patch
+---
+
+Fixes missing `<li>` wrapper when `href` prop is passed in `ListboxItem`, `MenuItem`, and `PaginationItem` (#4147)
diff --git a/packages/components/listbox/__tests__/listbox.test.tsx b/packages/components/listbox/__tests__/listbox.test.tsx
@@ -124,6 +124,40 @@ describe("Listbox", () => {
     expect(() => wrapper.unmount()).not.toThrow();
   });
 
+  it("should not have anchor tag when href prop is not passed", () => {
+    render(
+      <Listbox disallowEmptySelection aria-label="Actions" selectionMode="multiple">
+        <ListboxItem key="new">New file</ListboxItem>
+        <ListboxItem key="copy">Copy link</ListboxItem>
+        <ListboxItem key="edit">Edit file</ListboxItem>
+      </Listbox>,
+    );
+
+    let anchorTag = document.getElementsByTagName("a")[0];
+
+    expect(anchorTag).toBeFalsy();
+  });
+
+  it("should have anchor tag when href prop is passed", () => {
+    const href = "http://www.nextui.org/";
+
+    render(
+      <Listbox disallowEmptySelection aria-label="Actions" selectionMode="multiple">
+        <ListboxItem key="new" href={href}>
+          New file
+        </ListboxItem>
+        <ListboxItem key="copy">Copy link</ListboxItem>
+        <ListboxItem key="edit">Edit file</ListboxItem>
+      </Listbox>,
+    );
+
+    let anchorTag = document.getElementsByTagName("a")[0];
+
+    expect(anchorTag).toBeTruthy();
+
+    expect(anchorTag).toHaveProperty("href", href);
+  });
+
   it("should work with single selection (controlled)", async () => {
     let onSelectionChange = jest.fn();
 

diff --git a/packages/components/listbox/src/listbox-item.tsx b/packages/components/listbox/src/listbox-item.tsx
@@ -12,6 +12,7 @@ export interface ListboxItemProps<T extends object = object> extends UseListboxI
 const ListboxItem = forwardRef<"li", ListboxItemProps>((props, _) => {
   const {
     Component,
+    FragmentWrapper,
     rendered,
     description,
     isSelectable,
@@ -22,6 +23,7 @@ const ListboxItem = forwardRef<"li", ListboxItemProps>((props, _) => {
     endContent,
     hideSelectedIcon,
     disableAnimation,
+    fragmentWrapperProps,
     getItemProps,
     getLabelProps,
     getWrapperProps,
@@ -45,19 +47,21 @@ const ListboxItem = forwardRef<"li", ListboxItemProps>((props, _) => {
 
   return (
     <Component {...getItemProps()}>
-      {startContent}
-      {description ? (
-        <div {...getWrapperProps()}>
+      <FragmentWrapper {...fragmentWrapperProps}>
+        {startContent}
+        {description ? (
+          <div {...getWrapperProps()}>
+            <span {...getLabelProps()}>{rendered}</span>
+            <span {...getDescriptionProps()}>{description}</span>
+          </div>
+        ) : (
           <span {...getLabelProps()}>{rendered}</span>
-          <span {...getDescriptionProps()}>{description}</span>
-        </div>
-      ) : (
-        <span {...getLabelProps()}>{rendered}</span>
-      )}
-      {isSelectable && !hideSelectedIcon && (
-        <span {...getSelectedIconProps()}>{selectedContent}</span>
-      )}
-      {endContent}
+        )}
+        {isSelectable && !hideSelectedIcon && (
+          <span {...getSelectedIconProps()}>{selectedContent}</span>
+        )}
+        {endContent}
+      </FragmentWrapper>
     </Component>
   );
 });

diff --git a/packages/components/listbox/src/use-listbox-item.ts b/packages/components/listbox/src/use-listbox-item.ts
@@ -1,6 +1,6 @@
 import type {ListboxItemBaseProps} from "./base/listbox-item-base";
 
-import {useMemo, useRef, useCallback} from "react";
+import {useMemo, useRef, useCallback, Fragment} from "react";
 import {listboxItem} from "@nextui-org/theme";
 import {
   HTMLNextUIProps,
@@ -48,6 +48,7 @@ export function useListboxItem<T extends object>(originalProps: UseListboxItemPr
     shouldHighlightOnFocus,
     hideSelectedIcon = false,
     isReadOnly = false,
+    href,
     ...otherProps
   } = props;
 
@@ -56,9 +57,12 @@ export function useListboxItem<T extends object>(originalProps: UseListboxItemPr
 
   const domRef = useRef<HTMLLIElement>(null);
 
-  const Component = as || (originalProps.href ? "a" : "li");
+  const Component = as || "li";
   const shouldFilterDOMProps = typeof Component === "string";
 
+  const FragmentWrapper = href ? "a" : Fragment;
+  const fragmentWrapperProps = href ? {href} : {};
+
   const {rendered, key} = item;
 
   const isDisabled = state.disabledKeys.has(key) || originalProps.isDisabled;
@@ -167,6 +171,7 @@ export function useListboxItem<T extends object>(originalProps: UseListboxItemPr
 
   return {
     Component,
+    FragmentWrapper,
     domRef,
     slots,
     classNames,
@@ -180,6 +185,7 @@ export function useListboxItem<T extends object>(originalProps: UseListboxItemPr
     selectedIcon,
     hideSelectedIcon,
     disableAnimation,
+    fragmentWrapperProps,
     getItemProps,
     getLabelProps,
     getWrapperProps,

diff --git a/packages/components/menu/__tests__/menu.test.tsx b/packages/components/menu/__tests__/menu.test.tsx
@@ -125,6 +125,46 @@ describe("Menu", () => {
     expect(() => wrapper.unmount()).not.toThrow();
   });
 
+  it("should not have anchor tag when href prop is not passed", () => {
+    render(
+      <Menu disallowEmptySelection aria-label="Actions" selectionMode="multiple">
+        <MenuItem key="new">New file</MenuItem>
+        <MenuItem key="copy">Copy link</MenuItem>
+        <MenuItem key="edit">Edit file</MenuItem>
+        <MenuItem key="delete" color="danger">
+          Delete file
+        </MenuItem>
+      </Menu>,
+    );
+
+    let anchorTag = document.getElementsByTagName("a")[0];
+
+    expect(anchorTag).toBeFalsy();
+  });
+
+  it("should have anchor tag when href prop is passed", () => {
+    const href = "http://www.nextui.org/";
+
+    render(
+      <Menu disallowEmptySelection aria-label="Actions" selectionMode="multiple">
+        <MenuItem key="new" href={href}>
+          New file
+        </MenuItem>
+        <MenuItem key="copy">Copy link</MenuItem>
+        <MenuItem key="edit">Edit file</MenuItem>
+        <MenuItem key="delete" color="danger">
+          Delete file
+        </MenuItem>
+      </Menu>,
+    );
+
+    let anchorTag = document.getElementsByTagName("a")[0];
+
+    expect(anchorTag).toBeTruthy();
+
+    expect(anchorTag).toHaveProperty("href", href);
+  });
+
   it("should work with single selection (controlled)", async () => {
     let onSelectionChange = jest.fn();
 

diff --git a/packages/components/menu/src/menu-item.tsx b/packages/components/menu/src/menu-item.tsx
@@ -1,5 +1,6 @@
 import {useMemo, ReactNode} from "react";
 import {forwardRef} from "@nextui-org/system";
+import * as React from "react";
 
 import {UseMenuItemProps, useMenuItem} from "./use-menu-item";
 import {MenuSelectedIcon} from "./menu-selected-icon";
@@ -12,6 +13,7 @@ export interface MenuItemProps<T extends object = object> extends UseMenuItemPro
 const MenuItem = forwardRef<"li", MenuItemProps>((props, _) => {
   const {
     Component,
+    FragmentWrapper,
     slots,
     classNames,
     rendered,
@@ -25,6 +27,7 @@ const MenuItem = forwardRef<"li", MenuItemProps>((props, _) => {
     endContent,
     disableAnimation,
     hideSelectedIcon,
+    fragmentWrapperProps,
     getItemProps,
     getLabelProps,
     getDescriptionProps,
@@ -48,20 +51,22 @@ const MenuItem = forwardRef<"li", MenuItemProps>((props, _) => {
 
   return (
     <Component {...getItemProps()}>
-      {startContent}
-      {description ? (
-        <div className={slots.wrapper({class: classNames?.wrapper})}>
+      <FragmentWrapper {...fragmentWrapperProps}>
+        {startContent}
+        {description ? (
+          <div className={slots.wrapper({class: classNames?.wrapper})}>
+            <span {...getLabelProps()}>{rendered}</span>
+            <span {...getDescriptionProps()}>{description}</span>
+          </div>
+        ) : (
           <span {...getLabelProps()}>{rendered}</span>
-          <span {...getDescriptionProps()}>{description}</span>
-        </div>
-      ) : (
-        <span {...getLabelProps()}>{rendered}</span>
-      )}
-      {shortcut && <kbd {...getKeyboardShortcutProps()}>{shortcut}</kbd>}
-      {isSelectable && !hideSelectedIcon && (
-        <span {...getSelectedIconProps()}>{selectedContent}</span>
-      )}
-      {endContent}
+        )}
+        {shortcut && <kbd {...getKeyboardShortcutProps()}>{shortcut}</kbd>}
+        {isSelectable && !hideSelectedIcon && (
+          <span {...getSelectedIconProps()}>{selectedContent}</span>
+        )}
+        {endContent}
+      </FragmentWrapper>
     </Component>
   );
 });

diff --git a/packages/components/menu/src/use-menu-item.ts b/packages/components/menu/src/use-menu-item.ts
@@ -1,7 +1,7 @@
 import type {MenuItemBaseProps} from "./base/menu-item-base";
 import type {Node} from "@react-types/shared";
 
-import {useMemo, useRef, useCallback} from "react";
+import {useMemo, useRef, useCallback, Fragment} from "react";
 import {menuItem} from "@nextui-org/theme";
 import {
   HTMLNextUIProps,
@@ -57,6 +57,7 @@ export function useMenuItem<T extends object>(originalProps: UseMenuItemProps<T>
     isReadOnly = false,
     closeOnSelect,
     onClose,
+    href,
     ...otherProps
   } = props;
 
@@ -65,9 +66,12 @@ export function useMenuItem<T extends object>(originalProps: UseMenuItemProps<T>
 
   const domRef = useRef<HTMLLIElement>(null);
 
-  const Component = as || (otherProps?.href ? "a" : "li");
+  const Component = as || "li";
   const shouldFilterDOMProps = typeof Component === "string";
 
+  const FragmentWrapper = href ? "a" : Fragment;
+  const fragmentWrapperProps = href ? {href} : {};
+
   const {rendered, key} = item;
 
   const isDisabledProp = state.disabledKeys.has(key) || originalProps.isDisabled;
@@ -192,6 +196,7 @@ export function useMenuItem<T extends object>(originalProps: UseMenuItemProps<T>
 
   return {
     Component,
+    FragmentWrapper,
     domRef,
     slots,
     classNames,
@@ -205,6 +210,7 @@ export function useMenuItem<T extends object>(originalProps: UseMenuItemProps<T>
     endContent,
     selectedIcon,
     disableAnimation,
+    fragmentWrapperProps,
     getItemProps,
     getLabelProps,
     hideSelectedIcon,

diff --git a/packages/components/pagination/__tests__/pagination.test.tsx b/packages/components/pagination/__tests__/pagination.test.tsx
@@ -1,7 +1,7 @@
 import * as React from "react";
 import {render} from "@testing-library/react";
 
-import {Pagination} from "../src";
+import {Pagination, PaginationItem} from "../src";
 
 describe("Pagination", () => {
   it("should render correctly", () => {
@@ -37,6 +37,25 @@ describe("Pagination", () => {
     expect(prevButton).toBeNull();
   });
 
+  it("should not have anchor tag when href prop is not passed", () => {
+    render(<PaginationItem />);
+    let anchorTag = document.getElementsByTagName("a")[0];
+
+    expect(anchorTag).toBeFalsy();
+  });
+
+  it("should have anchor tag when href prop is passed", () => {
+    const href = "http://www.nextui.org/";
+
+    render(<PaginationItem href={href} />);
+
+    let anchorTag = document.getElementsByTagName("a")[0];
+
+    expect(anchorTag).toBeTruthy();
+
+    expect(anchorTag).toHaveProperty("href", href);
+  });
+
   it("should show dots when total is greater than 10", () => {
     const wrapper = render(<Pagination total={10} />);
 

diff --git a/packages/components/pagination/src/pagination-item.tsx b/packages/components/pagination/src/pagination-item.tsx
@@ -5,9 +5,14 @@ import {usePaginationItem, UsePaginationItemProps} from "./use-pagination-item";
 export interface PaginationItemProps extends UsePaginationItemProps {}
 
 const PaginationItem = forwardRef<"li", PaginationItemProps>((props, ref) => {
-  const {Component, children, getItemProps} = usePaginationItem({...props, ref});
+  const {Component, FragmentWrapper, fragmentWrapperProps, children, getItemProps} =
+    usePaginationItem({...props, ref});
 
-  return <Component {...getItemProps()}>{children}</Component>;
+  return (
+    <Component {...getItemProps()}>
+      <FragmentWrapper {...fragmentWrapperProps}>{children}</FragmentWrapper>
+    </Component>
+  );
 });
 
 PaginationItem.displayName = "NextUI.PaginationItem";

diff --git a/packages/components/pagination/src/use-pagination-item.ts b/packages/components/pagination/src/use-pagination-item.ts
@@ -2,7 +2,7 @@ import type {Ref} from "react";
 import type {HTMLNextUIProps, PropGetter} from "@nextui-org/system";
 import type {LinkDOMProps, PressEvent} from "@react-types/shared";
 
-import {useMemo} from "react";
+import {Fragment, useMemo} from "react";
 import {PaginationItemValue} from "@nextui-org/use-pagination";
 import {clsx, dataAttr} from "@nextui-org/shared-utils";
 import {chain, mergeProps, shouldClientNavigate, useRouter} from "@react-aria/utils";
@@ -64,10 +64,13 @@ export function usePaginationItem(props: UsePaginationItemProps) {
   } = props;
 
   const isLink = !!props?.href;
-  const Component = as || isLink ? "a" : "li";
+  const Component = as || "li";
   const shouldFilterDOMProps = typeof Component === "string";
-  const domRef = useDOMRef(ref);
 
+  const FragmentWrapper = isLink ? "a" : Fragment;
+  const fragmentWrapperProps = isLink ? {href: props.href} : {};
+
+  const domRef = useDOMRef(ref);
   const router = useRouter();
 
   const ariaLabel = useMemo(
@@ -129,6 +132,8 @@ export function usePaginationItem(props: UsePaginationItemProps) {
 
   return {
     Component,
+    FragmentWrapper,
+    fragmentWrapperProps,
     children,
     ariaLabel,
     isFocused,