fix: missing li tag when href is specified

[macci001]

Closes #4147

📝 Description

Fixes missing <li> wrapper when href prop is passed in ListboxItem, MenuItem, and PaginationItem

⛳️ Current behavior (updates)

The <li> element was missing when href prop was passed.

🚀 New behavior

<li> element wraps the <a> element.

Summary by CodeRabbit

• New Features

  • Introduced FragmentWrapper prop in ListboxItem, MenuItem, and PaginationItem components for improved rendering structure.
  • Added support for href property in ListboxItem, MenuItem, and PaginationItem, allowing these components to render as links when applicable.

• Bug Fixes

  • Resolved missing <li> wrapper issue in ListboxItem, MenuItem, and PaginationItem when using the href property.

• Enhancements

  • Improved flexibility in rendering child components within pagination items and menu items.

[linear]

ENG-1612 [BUG] - DropdownItem missing <li> tag when using href

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nextui-docs-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 29, 2024 2:09pm
nextui-storybook-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 29, 2024 2:09pm

[changeset-bot]

🦋 Changeset detected

Latest commit: bd28852

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@nextui-org/listbox	Patch
@nextui-org/menu	Patch
@nextui-org/pagination	Patch
@nextui-org/autocomplete	Patch
@nextui-org/select	Patch
@nextui-org/react	Patch
@nextui-org/dropdown	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (5)

• main
• canary
• fix/.*
• chore/.*
• feat/.*

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces a patch for the ListboxItem, MenuItem, and PaginationItem components from the @nextui-org library. The primary change ensures that when the href property is used, the components properly render a <li> wrapper around the <a> tag, thus maintaining the correct HTML structure. Additionally, new props such as FragmentWrapper and fragmentWrapperProps are introduced to enhance the rendering logic and flexibility of these components.

Changes

File Path	Change Summary
.changeset/nasty-bees-sniff.md	Introduced a patch for ListboxItem, MenuItem, and PaginationItem to include <li> wrappers when href is used.
packages/components/listbox/src/listbox-item.tsx	Added FragmentWrapper prop to ListboxItem, updated rendering logic to wrap content within FragmentWrapper.
packages/components/listbox/src/use-listbox-item.ts	Enhanced useListboxItem hook by adding href prop and adjusting rendering logic to conditionally use FragmentWrapper.
packages/components/menu/src/menu-item.tsx	Added FragmentWrapper prop to MenuItem, modified rendering structure to wrap content within FragmentWrapper.
packages/components/menu/src/use-menu-item.ts	Enhanced useMenuItem hook by adding href prop and adjusting rendering logic to conditionally use FragmentWrapper.
packages/components/pagination/src/pagination-item.tsx	Updated PaginationItem to include FragmentWrapper and fragmentWrapperProps in its rendering logic.
packages/components/pagination/src/use-pagination-item.ts	Enhanced usePaginationItem hook by adding FragmentWrapper and fragmentWrapperProps, allowing for flexible rendering based on the href prop.

Assessment against linked issues

Objective	Addressed	Explanation
The DropdownItem should render an <li> element that wraps the <a> tag (#4147)	✅

Possibly related PRs

• fix(system): missing useHref logic #2943: Fixes missing useHref logic in the @nextui-org/system package, relevant to link handling in components.
• fix(use-aria-menu): link logic in useMenuItem #3229: Addresses link logic in useMenuItem, directly related to changes in the MenuItem component.
• fix: RA dependencies bump #3240: Updates React Aria dependencies, potentially impacting the functionality of list components.
• fix(listbox): card shadow clipping when inside listbox item #3486: Fixes card shadow clipping in ListboxItem, relevant to changes in the ListboxItem component.
• fix(theme): add truncate class to the list item to avoid overflow the wrapper #4105: Addresses overflow text in menu items, relevant to MenuItem changes.
• fix: menu item classNames not work #4156: Fixes class name application issues in menu items, directly related to changes in the MenuItem component.

Suggested labels

📋 Scope : Docs, 👀 Status: To Review

Suggested reviewers

• wingkwong
• jrgarciadev

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (6)

.changeset/nasty-bees-sniff.md (1)

7-7: Consider enhancing the changeset description.

While the current description is clear, it could be more helpful to future readers by including:

• The impact of the fix (improves HTML semantics)
• The affected component behavior (wraps <a> tags with <li> when href is used)

Consider expanding it to:

-Fixes missing `<li>` wrapper when `href` prop is passed in `ListboxItem`, `MenuItem`, and `PaginationItem` (#4147)
+Fixes missing `<li>` wrapper when `href` prop is passed in `ListboxItem`, `MenuItem`, and `PaginationItem` to maintain proper HTML semantics. When `href` is provided, the components now correctly wrap the `<a>` tag with an `<li>` element. (#4147)

packages/components/listbox/src/listbox-item.tsx (1)

Line range hint 1-67: Consider adding specific test cases for href functionality.

Since this is fixing a specific issue with href rendering, we should ensure proper test coverage for this scenario.

Consider adding test cases that verify:

1. Proper <li> wrapper when href is provided
2. Correct attribute forwarding to the anchor element
3. Nested structure maintains accessibility

Would you like me to help generate these test cases?

packages/components/menu/src/menu-item.tsx (1)

16-16: Document the new wrapper props

The addition of FragmentWrapper and fragmentWrapperProps looks good and aligns with the PR objective of ensuring proper HTML structure. However, these props should be documented in the interface.

Consider adding JSDoc comments to MenuItemProps interface:

export interface MenuItemProps<T extends object = object> extends UseMenuItemProps<T> {
  /**
   * Component to wrap the menu item content when using href
   * @default Fragment
   */
  FragmentWrapper?: React.ComponentType<any>;
  
  /**
   * Props to pass to the fragment wrapper
   */
  fragmentWrapperProps?: Record<string, any>;
}

Also applies to: 30-30

packages/components/pagination/src/use-pagination-item.ts (2)

135-136: Consider adding JSDoc comments for the new return properties

The new properties are correctly added to the return object. Consider adding JSDoc comments to document their purpose and usage.

Add documentation like this:

 return {
   Component,
+  /** The wrapper component to use for link/non-link scenarios */
   FragmentWrapper,
+  /** Props to be spread on the FragmentWrapper component */
   fragmentWrapperProps,
   children,

---

67-71: Well-architected solution that maintains semantic HTML

The combination of simplified Component assignment and conditional FragmentWrapper creates a robust solution that:

1. Ensures proper semantic HTML with li wrapping
2. Correctly handles both link and non-link cases
3. Maintains flexibility through prop overrides
4. Minimizes DOM overhead by using Fragment when possible

This implementation serves as a good pattern for similar components requiring conditional link wrapping.

packages/components/listbox/src/use-listbox-item.ts (1)

63-64: Consider adding href validation and explicit typing

While the implementation works, consider these improvements:

1. Add runtime validation for the href value
2. Add explicit type for fragmentWrapperProps

+ type FragmentWrapperProps = { href?: string };
- const fragmentWrapperProps = href ? {href} : {};
+ const fragmentWrapperProps: FragmentWrapperProps = href ? {
+   href: typeof href === 'string' ? href : String(href)
+ } : {};

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ad79545 and 57fb87a.

📒 Files selected for processing (7)

• .changeset/nasty-bees-sniff.md (1 hunks)
• packages/components/listbox/src/listbox-item.tsx (3 hunks)
• packages/components/listbox/src/use-listbox-item.ts (5 hunks)
• packages/components/menu/src/menu-item.tsx (4 hunks)
• packages/components/menu/src/use-menu-item.ts (5 hunks)
• packages/components/pagination/src/pagination-item.tsx (1 hunks)
• packages/components/pagination/src/use-pagination-item.ts (3 hunks)

🔇 Additional comments (20)

.changeset/nasty-bees-sniff.md (1)

1-5: LGTM! Version bumps follow semantic versioning.

The patch version bumps are appropriate for this bug fix across the affected packages.

packages/components/pagination/src/pagination-item.tsx (2)

8-9: LGTM: Clean implementation of the fragment wrapper pattern

The addition of FragmentWrapper and fragmentWrapperProps from the hook aligns well with the PR's objective to fix semantic HTML structure.

---

11-15: Verify the semantic HTML output

The implementation looks correct, but let's verify that it properly wraps links in <li> elements when the href prop is used.

✅ Verification successful

Semantic HTML structure is properly implemented

From the code analysis, we can confirm that:

• The main Pagination component is rendered as a <nav> element containing a <ul> list
• Each PaginationItem is forwarded with a <li> ref, ensuring proper list semantics
• The test suite confirms the component renders correctly and includes accessibility attributes
• The implementation properly handles navigation controls and interactive elements

The semantic structure follows the correct HTML hierarchy: nav > ul > li, which is the recommended pattern for pagination components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the component renders proper semantic HTML with href prop
# Look for test files that verify the HTML structure

# Search for relevant test files
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | grep -i "pagination"

# Search for semantic HTML structure tests
rg -l "href.*li|li.*href" --type ts --type tsx

# Look for existing snapshot tests
rg "toMatchSnapshot|render.*href" --type ts --type tsx -A 5

Length of output: 365

---

Script:

#!/bin/bash
# Let's examine the test file content and implementation details

# Check the test file content
cat "packages/components/pagination/__tests__/pagination.test.tsx"

# Look for component implementation
cat "packages/components/pagination/src/pagination-item.tsx"

# Check if there are any snapshots
fd -e snap -e snapshot . | grep -i pagination

Length of output: 2760

---

Script:

#!/bin/bash
# Let's check the implementation of usePaginationItem hook to understand the semantic structure

# Check the hook implementation
cat "packages/components/pagination/src/use-pagination-item.tsx"

# Look for any additional test files that might test the PaginationItem component
fd -e test.tsx -e spec.tsx . | grep -i "pagination-item"

Length of output: 212

---

Script:

#!/bin/bash
# Let's check the hook implementation from the hooks package and the main pagination implementation

# Check if the hook is in the hooks package
cat "packages/hooks/use-pagination/src/use-pagination-item.ts"

# Check the main pagination implementation to understand the structure
cat "packages/components/pagination/src/pagination.tsx"

Length of output: 6336

packages/components/listbox/src/listbox-item.tsx (2)

15-15: LGTM! Props addition aligns with requirements.

The new FragmentWrapper and fragmentWrapperProps additions provide the necessary flexibility to handle both anchor and non-anchor cases while maintaining proper semantic structure.

Also applies to: 26-26

---

50-64: Verify semantic HTML structure when href is used.

The new wrapper implementation looks correct, but we should verify that it produces the expected HTML structure: <li><a>...</a></li> when href is provided.

✅ Verification successful

Let me gather more information about the HTML structure by checking the implementation and tests.

---

Let me check the specific test file and implementation for the Listbox component to verify the HTML structure.

---

Let me check one final thing to verify the HTML structure when href is used.

---

Based on the code analysis, I can now provide a final response:

The HTML structure is correctly implemented for href usage

The implementation in use-listbox-item.ts shows that when href is provided:

• The FragmentWrapper is set to "a" element (const FragmentWrapper = href ? "a" : Fragment)
• The href prop is properly passed through fragmentWrapperProps
• The wrapper maintains the correct semantic structure as the anchor is wrapped within the list item

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test files related to ListboxItem to verify href rendering
fd -e test.tsx -e spec.tsx listbox-item

# Search for existing href usage patterns
rg -A 2 "href.*ListboxItem|ListboxItem.*href"

Length of output: 1246

---

Script:

#!/bin/bash
# Search for the implementation of FragmentWrapper and related props
ast-grep --pattern 'const FragmentWrapper = $_'

# Search for test files that might verify the HTML structure
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | xargs rg -l "render|screen.getBy"

# Look for any existing snapshot tests
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | xargs rg "toMatchSnapshot|toMatchInlineSnapshot"

Length of output: 3502

---

Script:

#!/bin/bash
# Check the listbox test file content
cat packages/components/listbox/__tests__/listbox.test.tsx

# Look for the implementation of use-listbox-item
cat packages/components/listbox/src/use-listbox-item.ts

# Search for any render tests specifically mentioning href
rg -A 5 "href.*render|render.*href" packages/components/listbox/__tests__/listbox.test.tsx

Length of output: 14060

---

Script:

#!/bin/bash
# Add a test case for href and check the rendered HTML structure
echo "
import * as React from 'react';
import {render} from '@testing-library/react';
import {Listbox, ListboxItem} from '../src';

describe('ListboxItem with href', () => {
  it('should render with correct HTML structure', () => {
    const {container} = render(
      <Listbox>
        <ListboxItem href='/test'>Test Link</ListboxItem>
      </Listbox>
    );
    console.log(container.innerHTML);
  });
});
" > temp-test.tsx

# Look for any existing DOM structure tests
rg -A 3 "getByRole.*listbox|container\.querySelector" packages/components/listbox/__tests__/

Length of output: 1155

packages/components/menu/src/menu-item.tsx (2)

54-69: LGTM! Verify accessibility with screen readers

The implementation correctly wraps the content while maintaining all existing functionality. This should properly handle the href case by ensuring correct semantic HTML structure.

✅ Verification successful

Let me gather more information about the accessibility implementation, particularly focusing on the FragmentWrapper component and menu item accessibility attributes.

---

Let me check one more aspect of the accessibility implementation, particularly the menu item base component and its ARIA attributes.

---

Implementation maintains proper accessibility attributes and structure

The FragmentWrapper implementation correctly preserves accessibility by:

• Using semantic HTML (<a> tag) when href is provided
• Maintaining proper ARIA attributes through dedicated prop getters
• Ensuring screen reader accessibility with proper labeling and descriptions
• Hiding decorative elements appropriately (selectedIcon has aria-hidden)

The structure follows accessibility best practices with:

• Proper keyboard shortcut support via <kbd> element
• Semantic grouping of label and description when both are present
• Proper ARIA attributes inheritance through the component hierarchy

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for proper ARIA attributes and roles in similar components
rg -A 2 'aria-|role=' packages/components/menu/src/

Length of output: 1704

---

Script:

#!/bin/bash
# Check FragmentWrapper implementation and menu item accessibility
ast-grep --pattern 'const FragmentWrapper = $_'

# Check menu item related hooks and base components for accessibility
rg -A 3 'role=|aria-' packages/components/menu/src/use-menu.ts
rg -A 3 'role=|aria-' packages/components/menu/src/base/menu-base.tsx

Length of output: 588

---

Script:

#!/bin/bash
# Check menu item accessibility implementation
rg -A 5 'getLabelProps|getDescriptionProps|getKeyboardShortcutProps|getSelectedIconProps' packages/components/menu/src/
ast-grep --pattern 'function useMenuItem($_) { $$$ }'

Length of output: 4310

---

3-3: Verify necessity of additional React import

The file already imports specific React exports (useMemo, ReactNode). Let's verify if this additional import is necessary.

packages/components/pagination/src/use-pagination-item.ts (3)

5-5: LGTM: Fragment import is correctly added

The Fragment import is necessary for the new wrapper logic and is properly placed with other React imports.

---

67-67: LGTM: Simplified Component assignment improves clarity

The simplified assignment ensures consistent li wrapping while maintaining flexibility through the as prop, which aligns well with the PR's objective.

---

70-71: LGTM: Elegant solution for conditional link wrapping

The implementation elegantly handles both link and non-link cases while maintaining proper HTML semantics. Let's verify this solution is consistently applied across similar components.

✅ Verification successful

Consistent implementation verified across interactive components

The pattern is correctly and consistently implemented across similar interactive components:

• pagination/use-pagination-item.ts: Uses isLink ? "a" : Fragment
• menu/use-menu-item.ts: Uses href ? "a" : Fragment
• listbox/use-listbox-item.ts: Uses href ? "a" : Fragment

The minor variation in the condition (isLink vs direct href check) doesn't impact the functionality as they achieve the same result.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if similar components implement the same pattern

# Search for similar wrapper implementations in other components
ast-grep --pattern 'const FragmentWrapper = $_ ? "a" : Fragment'

# Check for potential inconsistencies in link handling
rg -A 3 'isLink.*=.*!!.*href' packages/components/

Length of output: 792

packages/components/listbox/src/use-listbox-item.ts (5)

3-3: LGTM: Fragment import added correctly

The Fragment import is necessary for the new wrapper logic and is properly placed with other React imports.

---

51-51: LGTM: href prop added correctly

The href prop addition aligns with the PR objectives and is properly typed through the Props interface inheritance.

---

60-60: LGTM: Component properly defaults to "li"

The Component assignment ensures proper semantic structure by always using "li" unless explicitly overridden.

---

Line range hint 172-186: LGTM: Return type properly extended

The addition of FragmentWrapper and fragmentWrapperProps to the return object is consistent and well-structured.

---

Line range hint 172-186: Verify the implementation in consumer components

Let's ensure that all components using this hook properly implement the new wrapper logic.

✅ Verification successful

Implementation verified across consumer components

The FragmentWrapper and related props are correctly implemented across all consumer components:

• The ListboxItem component properly uses FragmentWrapper with fragmentWrapperProps
• Similar pattern is consistently used in other components:
  • MenuItem in packages/components/menu
  • PaginationItem in packages/components/pagination

All implementations follow the same pattern where FragmentWrapper is conditionally set to either an anchor tag or Fragment based on the presence of href, and the wrapper is properly used with its props.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for components using useListboxItem
ast-grep --pattern 'const {$$$} = useListboxItem($$$)'

# Search for potential direct FragmentWrapper usage
rg "FragmentWrapper" --type typescript

Length of output: 1557

---

Script:

#!/bin/bash
# Search for FragmentWrapper with ripgrep (without type filter)
rg "FragmentWrapper"

# Let's also check the implementation of ListboxItem to see how these props are used
cat packages/components/listbox/src/listbox-item.tsx

Length of output: 3264

packages/components/menu/src/use-menu-item.ts (5)

4-4: LGTM: Fragment import added correctly

The Fragment import is appropriately added alongside other React imports to support the conditional rendering logic.

---

60-62: LGTM: href prop correctly added

The href prop is appropriately destructured from props and inherits its type from HTMLNextUIProps<"li">.

---

69-74: LGTM: Proper implementation of conditional wrapper

The implementation correctly handles the href case by using either Fragment or anchor tag, maintaining proper semantic HTML structure.

---

197-197: LGTM: Return type correctly updated

The FragmentWrapper and fragmentWrapperProps are appropriately added to the return object, maintaining type safety through TypeScript's inference.

Also applies to: 211-211

---

72-73: Verify accessibility implications of nested anchor tags

Please ensure that nesting anchor tags inside list items follows accessibility best practices and works correctly with screen readers.

✅ Verification successful

Nesting anchor tags is handled safely with Fragment fallback

The implementation is accessibility-safe for the following reasons:

• The component uses proper ARIA utilities (@nextui-org/use-aria-menu and @nextui-org/aria-utils)
• The FragmentWrapper pattern is a common approach also used in other accessible components (pagination, listbox)
• When href is not provided, it falls back to Fragment to avoid invalid nesting
• The component includes proper ARIA attributes (aria-label, aria-checked) and roles as shown in the tests

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar patterns in other accessible components
ast-grep --pattern 'const $wrapper = $condition ? "a" : Fragment'

# Search for accessibility tests
rg -l 'accessibility|aria|screen.?reader' --type=test

Length of output: 152

---

Script:

#!/bin/bash
# Search for accessibility tests with explicit file extensions
rg -l 'accessibility|aria|screen.?reader' --type=ts --type=tsx --type=js --type=jsx

# Search for anchor tag usage patterns in the menu component
rg -A 5 -B 5 '<a' packages/components/menu/

# Look for aria attributes in menu components
rg 'aria-' packages/components/menu/

# Search for Fragment usage patterns
ast-grep --pattern 'const $_ = $_ ? "a" : Fragment'

Length of output: 7230

---

Script:

#!/bin/bash
# Check for menu-related aria hooks and utilities
rg -l '@nextui-org/use-aria-menu|@nextui-org/aria-utils' packages/components/menu/

# Look for the implementation of useMenuItem hook
rg -A 10 'export.*useMenuItem' packages/components/menu/src/use-menu-item.ts

# Check for role attributes in menu components
rg 'role=' packages/components/menu/

Length of output: 815

[jrgarciadev]

Well done @macci001 thanks!