fix: reverting pr4168

[macci001]

• Closes [BUG] - <a> cannot appear as a descendant of <a> #4256
• Closes [BUG] - 2.6.2 DropdownItem links no longer function #4246
• Closes [BUG] - 2.6.2 DropdownMenu with startContent moves title to new line #4244

📝 Description

This reverts #4168

⛳️ Current behavior (updates)

• Links work fine now:

Screen.Recording.2024-12-09.at.11.45.21.AM.mov

• StartContent alignment and end content alignment looks good as well:
  
• There would be an issue if polymorphic component is used.

🚀 New behavior

💣 Is this a breaking change (Yes/No):

📝 Additional Information

Summary by CodeRabbit

Summary by CodeRabbit

• Bug Fixes

  • Removed tests for anchor tag rendering in ListboxItem, MenuItem, and PaginationItem components based on the href prop.

• New Features

  • Simplified rendering logic in ListboxItem, MenuItem, and PaginationItem components by removing unnecessary wrappers, enhancing readability and maintainability.

• Refactor

  • Streamlined component structures and logic in useListboxItem, useMenuItem, and usePaginationItem to improve clarity and reduce complexity.

• Chores

  • Patched packages: @nextui-org/pagination, @nextui-org/listbox, and @nextui-org/menu.
  • Reverted changes from PR-4168 related to issues [BUG] - <a> cannot appear as a descendant of <a> #4256, [BUG] - 2.6.2 DropdownItem links no longer function #4246, and [BUG] - 2.6.2 DropdownMenu with startContent moves title to new line #4244.

[changeset-bot]

🦋 Changeset detected

Latest commit: cdbd5f8

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/pagination	Patch
@nextui-org/listbox	Patch
@nextui-org/menu	Patch
@nextui-org/react	Patch
@nextui-org/autocomplete	Patch
@nextui-org/select	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

[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	Dec 9, 2024 7:06am
nextui-storybook-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 9, 2024 7:06am

[coderabbitai]

Walkthrough

The changes in this pull request primarily involve the removal of test cases related to the rendering of anchor tags in the ListboxItem, MenuItem, and PaginationItem components. Additionally, the FragmentWrapper has been eliminated from several components, which simplifies their structure and rendering process. The handling of the href prop has been updated to conditionally render either an anchor or a list item based on its presence, enhancing the clarity and maintainability of the code.

Changes

File Path	Change Summary
packages/components/listbox/__tests__/listbox.test.tsx	Removed tests for anchor tag rendering based on href prop.
packages/components/listbox/src/listbox-item.tsx	Removed FragmentWrapper, simplified rendering logic for description, startContent, and endContent.
packages/components/listbox/src/use-listbox-item.ts	Updated logic to conditionally render anchor or list item based on href, removed FragmentWrapper.
packages/components/menu/__tests__/menu.test.tsx	Removed tests for anchor tag rendering based on href prop.
packages/components/menu/src/menu-item.tsx	Removed FragmentWrapper, simplified rendering logic for description and child components.
packages/components/menu/src/use-menu-item.ts	Updated logic to conditionally render anchor or list item based on href, removed FragmentWrapper.
packages/components/pagination/__tests__/pagination.test.tsx	Removed tests for anchor tag rendering based on href prop.
packages/components/pagination/src/pagination-item.tsx	Removed FragmentWrapper, simplified rendering logic.
packages/components/pagination/src/use-pagination-item.ts	Updated logic to conditionally render anchor or list item based on href, removed FragmentWrapper.

Assessment against linked issues

Objective	Addressed	Explanation
Ensure <a> tags do not nest within each other (#4256)	✅
Ensure DropdownItem links function correctly (#4246)	❌	The changes do not address the navigation issue with DropdownItems.
Ensure titles in DropdownMenu do not move to a new line (#4244)	❓	It is unclear if the changes resolve the layout issue with startContent.

Possibly related PRs

• fix: missing li tag when href is specified #4168: Fixes the missing <li> wrapper when the href prop is specified in ListboxItem, directly relating to the changes made.
• chore: undo pr3307 #3756: Undoes changes from a previous PR that may have affected dropdown and menu components, which could relate to the changes in this PR.
• fix: menu item classNames not work #4156: Fixes class name issues in the MenuItem component, which may relate to the overall structure and rendering of menu items.
• fix: forwardRef render functions not using ref #4198: Addresses issues with forwardRef render functions in various components, relevant to the changes in ListboxItem.
• fix(pagination): cursor position when hidden on init #4222: Addresses cursor positioning in the pagination component, which may relate to user interactions similar to those in the ListboxItem.

Suggested labels

👀 Status: In Review

Suggested reviewers

• jrgarciadev
• wingkwong

---

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2870b31 and cdbd5f8.

📒 Files selected for processing (1)

• .changeset/breezy-bobcats-destroy.md (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• .changeset/breezy-bobcats-destroy.md

---

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 (3)

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

48-60: Improved HTML structure with proper semantic nesting

The restructured rendering logic:

1. Maintains proper heading hierarchy with description
2. Preserves accessibility through semantic HTML
3. Keeps selection indicators properly positioned

Consider adding a comment documenting the structure's accessibility considerations.

Add a comment above the return statement explaining the semantic structure and accessibility considerations:

+ // Component structure maintains ARIA landmarks and semantic HTML hierarchy
+ // with proper nesting of labels, descriptions, and selection indicators
  return (

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

Line range hint 8-10: Architectural consistency across components

The removal of FragmentWrapper across all components creates a consistent pattern that:

1. Fixes HTML nesting issues (particularly with anchor tags)
2. Maintains accessibility and functionality
3. Simplifies the component structure

This architectural change should make the codebase more maintainable and reduce the likelihood of HTML nesting issues.

Also applies to: 48-60, 51-64

packages/components/pagination/__tests__/pagination.test.tsx (1)

4-4: Consider adding tests for the new component type determination.

While removing tests related to the old approach is correct, consider adding new tests to verify the component renders the appropriate element type based on props.

Example test cases:

it("should render as li when no href is provided", () => {
  const wrapper = render(<PaginationItem />);
  expect(wrapper.container.querySelector("li")).toBeInTheDocument();
});

it("should render as anchor when href is provided", () => {
  const wrapper = render(<PaginationItem href="#" />);
  expect(wrapper.container.querySelector("a")).toBeInTheDocument();
});

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3b390fb and 2870b31.

📒 Files selected for processing (9)

• packages/components/listbox/__tests__/listbox.test.tsx (0 hunks)
• packages/components/listbox/src/listbox-item.tsx (1 hunks)
• packages/components/listbox/src/use-listbox-item.ts (2 hunks)
• packages/components/menu/__tests__/menu.test.tsx (0 hunks)
• packages/components/menu/src/menu-item.tsx (1 hunks)
• packages/components/menu/src/use-menu-item.ts (2 hunks)
• packages/components/pagination/__tests__/pagination.test.tsx (1 hunks)
• packages/components/pagination/src/pagination-item.tsx (1 hunks)
• packages/components/pagination/src/use-pagination-item.ts (2 hunks)

💤 Files with no reviewable changes (2)

• packages/components/listbox/tests/listbox.test.tsx
• packages/components/menu/tests/menu.test.tsx

🔇 Additional comments (7)

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

8-10: Simplified component structure improves HTML semantics

The removal of FragmentWrapper and direct rendering of Component helps prevent invalid HTML nesting issues. However, we should verify that this maintains proper accessibility attributes.

✅ Verification successful

Accessibility attributes are properly maintained after simplification

The verification shows that accessibility is well-maintained in the simplified structure:

• aria-label is dynamically set with proper active state indication
• aria-current is correctly managed for active items
• aria-disabled state is properly handled
• All aria attributes are applied through the getItemProps function

The removal of FragmentWrapper doesn't impact accessibility as all necessary ARIA attributes are still being passed to the rendered component.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if accessibility attributes are properly maintained
ast-grep --pattern 'aria-$_="$_"' packages/components/pagination/src/use-pagination-item.ts

Length of output: 93

---

Script:

#!/bin/bash
# Let's check the implementation of getItemProps to see how accessibility attributes are handled
ast-grep --pattern 'function getItemProps' -A 30 packages/components/pagination/src/use-pagination-item.ts

# Also check for any aria-related properties in the entire file
rg -i "aria" packages/components/pagination/src/use-pagination-item.ts

# Let's also check the test file for accessibility expectations
fd "pagination-item.test" --exec cat {}

Length of output: 796

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

51-64: Consistent implementation with ListboxItem pattern

The restructuring follows the same pattern as ListboxItem, which:

1. Maintains proper styling through slots
2. Preserves keyboard accessibility
3. Keeps consistent structure across components

However, we should verify that the styling remains intact with the new structure.

✅ Verification successful

Styling structure verified and properly maintained

The implementation correctly maintains the styling structure:

• The wrapper slot with className is properly implemented (slots.wrapper({class: classNames?.wrapper}))
• The pattern matches ListboxItem's slot usage for consistency
• All necessary class bindings are preserved in the restructured code

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if all className references are properly maintained
rg "slots\." packages/components/menu/src/menu-item.tsx -A 2

Length of output: 251

---

Script:

#!/bin/bash
# Let's check the base component (ListboxItem) for comparison
rg "slots\." packages/components/listbox/src/listbox-item.tsx -A 2

# Also check if there are any style-related tests for MenuItem
rg "className|slots" packages/components/menu/src/**/*test*

Length of output: 211

---

Script:

#!/bin/bash
# Let's check the base component (ListboxItem) for comparison with correct path
fd "listbox-item" --exec rg "slots\." {} -A 2

# Check for test files with correct path pattern
fd "test" --extension tsx --extension ts packages/components/menu/src/ --exec rg "className|slots" {}

# Check the slots definition in MenuItem
rg "const slots = " packages/components/menu/src/menu-item.tsx -A 5

Length of output: 617

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

Line range hint 108-127: LGTM! Robust click handling with proper router integration.

The click handler correctly chains events and manages client-side navigation while preserving the router's functionality.

---

67-70: LGTM! Component type determination fixes HTML nesting issues.

The simplified logic correctly determines whether to render an anchor or list item based on props, preventing invalid HTML nesting of <a> tags.

Let's verify there are no remaining nested anchor issues:

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

61-61: LGTM! Consistent component type determination.

The implementation mirrors the PaginationItem approach, ensuring proper HTML structure and preventing invalid nesting of anchor tags.

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

5-5: LGTM: Import statement cleanup

The import statement has been correctly simplified to include only the necessary React hooks.

---

70-70: Approve: Component type determination fixes HTML nesting issues

The simplified logic correctly determines when to use an anchor tag based on the presence of href, which addresses the HTML nesting issues reported in #4256.

Let's verify the fix handles all edge cases:

✅ Verification successful

Component type determination is correctly implemented

The verification confirms that the component type determination logic is correctly implemented and aligns with HTML nesting requirements:

• The logic as || (otherProps?.href ? "a" : "li") ensures proper HTML semantics by:
  • Using <a> only when href is present
  • Defaulting to <li> for standard menu items
  • Allowing custom component override via as prop

The implementation successfully prevents invalid HTML nesting while maintaining flexibility for custom routing components through the as prop.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential anchor tag nesting issues in tests and examples

# Search for test cases involving nested links or href props
rg -A 5 "as={Link}.*href" 

# Search for DropdownItem usage with href or Link
rg -A 5 "DropdownItem.*href|DropdownItem.*as=.*Link"

# Search for MenuItem usage with href or Link
rg -A 5 "MenuItem.*href|MenuItem.*as=.*Link"

Length of output: 5850

---

Script:

#!/bin/bash
# Let's fix the regex and search for more specific patterns

# Search for test cases involving Link components
rg -A 5 '"as".*"Link"|as=\{Link\}'

# Search for nested anchor scenarios in tests
rg -A 5 'render\(' packages/components/menu/src/

# Check MenuItem implementation and tests
fd "menu-item" --exec cat {}

Length of output: 37337