fix(select): pass portalContainer to the pop-over

[macci001]

Closes #3695
Closes #3571

📝 Description

Currently the select component do not pass the portalContainer prop to it's child pop-over.
The pop-over hence takes default value of portalContainer i.e document.body
Due to this (when parent of the select component is scrollable) the scroll closes the pop-over immediately due to the scroll.
This makes it impossible for the pop-over to open.
The PR adds the portalContainer prop to the pop-over.

⛳️ Current behavior (updates)

• When trying to open the select, after selecting an element after certain height, the parent scrolls which closes the pop-over.

Screen.Recording.2024-09-01.at.1.07.52.PM.mov

• The pop-over does not get added as a child of base slot of select

🚀 New behavior

• The select works.

Screen.Recording.2024-09-01.at.1.01.49.PM.mov

• The pop-over gets added as a child of base slot of select

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

Summary by CodeRabbit

Summary by CodeRabbit

• New Features

  • Introduced a portalContainer prop for the select component's pop-over, improving usability within scrollable parent components.
  • Added a previewHeight property to enhance visual consistency in documentation examples.

• Bug Fixes

  • Removed the items-center class from the select component's wrapper to adjust layout styling.

• Tests

  • Added a test case to ensure the select component remains open when the parent is scrollable, verifying improved functionality and user experience.

These changes collectively enhance the user experience by ensuring the pop-over remains accessible and functional.

[changeset-bot]

🦋 Changeset detected

Latest commit: ae0f5fa

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

This PR includes changesets to release 2 packages

Name	Type
@nextui-org/select	Patch
@nextui-org/react	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]

@macci001 is attempting to deploy a commit to the NextUI Inc Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

Walkthrough

This update enhances the select component by introducing a portalContainer prop for its child pop-over, allowing developers to specify a custom container. This change addresses usability issues with scrollable parent components, ensuring the pop-over remains accessible. Additionally, a new reference, baseRef, is added to manage a base HTML element, improving the component's interaction and accessibility without altering its core logic.

Changes

Files	Change Summary
packages/components/select/src/use-select.ts	Added portalContainer prop to manage pop-over behavior; introduced baseRef for improved element interaction.
packages/components/select/__tests__/select.test.tsx	Added a test case to ensure the select component remains open when the parent is scrollable.
apps/docs/content/components/select/open-state.ts	Removed items-center class from the wrapping div element, affecting layout styling.
apps/docs/content/docs/components/select.mdx	Added previewHeight property to CodeDemo component instances for visual consistency.

Assessment against linked issues

Objective	Addressed	Explanation
Select component auto closes in flex layout (#3695)	✅
Select scrolls page and immediately closes popover on click (#3571)	✅

Possibly related PRs

• fix(components): improve RTL support for select component #2041: Improves RTL support for the select component, which may relate to overall functionality enhancements.
• fix: popover-based focus behaviour #2854: Addresses focus behavior in popovers, relevant to the portalContainer prop changes.
• fix(popover): close popover on scroll #3414: Fixes popover behavior while scrolling, related to enhancements made in this PR.
• fix(select): prevent select from closing instantly when clicking on label of select #3568: Resolves issues with the select menu closing unexpectedly, relevant to the changes in this PR.
• fix(theme): color discrepancy in select #3663: Fixes color discrepancies in the select component, aligning with the main PR's focus on usability and visual consistency.

---

Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bf7fe71 and ae0f5fa.

Files selected for processing (5)

• .changeset/clean-eagles-clean.md (1 hunks)
• apps/docs/content/components/select/open-state.ts (1 hunks)
• apps/docs/content/docs/components/select.mdx (9 hunks)
• packages/components/select/tests/select.test.tsx (2 hunks)
• packages/components/select/src/use-select.ts (5 hunks)

Files skipped from review due to trivial changes (1)

• apps/docs/content/components/select/open-state.ts

Files skipped from review as they are similar to previous changes (3)

• .changeset/clean-eagles-clean.md
• packages/components/select/tests/select.test.tsx
• packages/components/select/src/use-select.ts

Additional context used

LanguageTool

apps/docs/content/docs/components/select.mdx

[uncategorized] ~122-~122: Did you mean: “By default,”?
Context: ...t="350px" /> ### Custom Selector Icon By default the select uses a chevron-down icon a...

(BY_DEFAULT_COMMA)

---

[uncategorized] ~176-~176: Did you mean: “By default,”?
Context: ...ht="350px" /> ### Custom Render Value By default the select will render the selected ite...

(BY_DEFAULT_COMMA)

---

[typographical] ~250-~250: It appears that a comma is missing.
Context: ...ing the renderValue property. In this example we are using the [Chip](/docs/component...

(DURING_THAT_TIME_COMMA)

Additional comments not posted (10)

apps/docs/content/docs/components/select.mdx (10)

48-48: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency.

---

57-57: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency.

---

63-63: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency.

---

73-73: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency.

---

80-80: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency.

---

84-84: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency. The increased height of "500px" may be necessary to accommodate the content of this specific code demo.

---

88-88: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency. The increased height of "550px" may be necessary to accommodate the content of this specific code demo.

---

92-92: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency. The increased height of "550px" may be necessary to accommodate the content of this specific code demo.

---

96-96: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency. The increased height of "500px" may be necessary to accommodate the content of this specific code demo.

---

102-102: LGTM!

Setting a fixed previewHeight for the code demo helps maintain visual consistency. The increased height of "500px" may be necessary to accommodate the content of this specific code demo.

---

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 generate interesting stats about this repository and render them as a table.
  -- @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

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f36df43 and 29de544.

Files selected for processing (2)

• .changeset/clean-eagles-clean.md (1 hunks)
• packages/components/select/src/use-select.ts (5 hunks)

Additional comments not posted (2)

packages/components/select/src/use-select.ts (2)

235-235: LGTM! The addition of baseRef enhances the component's functionality.

The new baseRef reference is correctly initialized, passed to getBaseProps, and included in the dependency array. This allows the base element to be managed using the reference and ensures that any changes to baseRef will trigger a re-computation of the base element props.

Also applies to: 384-384, 396-396

---

524-524: LGTM! Passing baseRef to getPopoverProps improves the popover functionality.

Assigning baseRef.current to the portalContainer prop allows the popover to be rendered in relation to the base element. Including baseRef in the dependency array ensures that any changes to baseRef will trigger a re-computation of the popover props.

Also applies to: 547-547

[wingkwong]

1. please add test
2. please share the code you showed in PR description

[macci001]

hi @wingkwong
The code used in the PR description:

<div>
  {
    [...Array(50).keys()].map((_, idx) => (
      <div className="p-4" key={idx}>{idx}</div>
    ))
  }
</div>
<Select className="w-[500px] fixed bottom-2" color={color} label="Favorite Animal">
  {
    animalsData.map((data) => (
      <SelectItem key={data.value} value={data.value}>{data.label}</SelectItem>
    ))
  }
</Select>

I think the fix to add the portalContainer, the UI with multiple selects misbehaves(when select1 is open, the click on select2 closes the select1 but does not open the select2 until one more click on it.)
I am currently looking into this.

[coderabbitai]

Actionable comments posted: 1

Outside diff range, codebase verification and nitpick comments (1)

packages/components/select/src/use-select.ts (1)

Line range hint 383-395: Review the getBaseProps function for potential improvements.

The getBaseProps function is correctly using the baseRef to set the ref property. However, consider using a more descriptive name for the data-slot attribute to enhance readability and maintainability.

- "data-slot": "base",
+ "data-slot": "select-base",

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 29de544 and a2a5468.

Files selected for processing (3)

• .changeset/clean-eagles-clean.md (1 hunks)
• packages/components/select/tests/select.test.tsx (2 hunks)
• packages/components/select/src/use-select.ts (5 hunks)

Additional comments not posted (3)

packages/components/select/src/use-select.ts (2)

234-234: Addition of baseRef is well-implemented.

The introduction of baseRef using useRef is correctly implemented and follows React best practices for managing references.

---

Line range hint 523-543: Ensure correct implementation of portalContainer prop.

The getPopoverProps function now correctly uses baseRef.current to dynamically set the portalContainer. This is a crucial fix that aligns with the PR's objectives to handle the pop-over behavior correctly. Ensure thorough testing to verify that this change behaves as expected across all use cases.

Run the following script to verify the correct implementation of portalContainer:

Verification successful

---

Verification Successful: Correct Implementation of portalContainer Prop

The portalContainer prop in the getPopoverProps function is correctly implemented using baseRef.current ?? undefined, as expected. This aligns with the review comment's approval and ensures the desired behavior for the pop-over functionality.

• Location: packages/components/select/src/use-select.ts

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correct implementation of `portalContainer` in `getPopoverProps`.

# Test: Search for the usage of `portalContainer`. Expect: Only occurrences with `baseRef.current`.
rg --type typescript -A 5 $'portalContainer: baseRef.current'

Length of output: 97

---

Script:

#!/bin/bash
# Description: Verify the correct implementation of `portalContainer` in `getPopoverProps`.

# Test: Search for the usage of `portalContainer`. Expect: Only occurrences with `baseRef.current`.
rg --type ts -A 5 $'portalContainer: baseRef.current'

Length of output: 538

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

656-695: New test case is well-implemented and relevant.

The test case "should not close when parent is scrollable" is well-implemented and directly addresses the issue described in the PR. It effectively tests the Select component's behavior in a scrollable environment, ensuring that the pop-over remains open as expected.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a2a5468 and bd7666b.

Files selected for processing (1)

• .changeset/clean-eagles-clean.md (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• .changeset/clean-eagles-clean.md

[macci001]

Update:

• Added the test for the change.
• Following code can be used to reproduce the issue. (first click will scroll rather than opening the select pop-over)

<div className="h-screen w-screen m-10">
        <Select
          aria-label="Favorite Animal"
          className="fixed bottom-2"
          data-testid="select"
          label="Favorite Animal"
        >
          {itemsData.map((item) => (
            <SelectItem key={item.id} value={item.label}>
              {item.label}
            </SelectItem>
          ))}
        </Select>
      </div>,

• The issue mentioned in comment seems to be resolved as we now useAriaOverlay hook.

// cc @wingkwong

[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	Sep 12, 2024 1:37pm
nextui-storybook-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 12, 2024 1:37pm

[wingkwong]

pr3698-issue.webm

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (3)

apps/docs/content/docs/components/select.mdx (3)

122-122: Consider adding a comma after "By default".

According to the static analysis hint, adding a comma after "By default" would improve the sentence structure.

-By default the select uses a `chevron-down` icon as the selector icon which rotates when the select is open.
+By default, the select uses a `chevron-down` icon as the selector icon which rotates when the select is open.

Tools

LanguageTool

[uncategorized] ~122-~122: Did you mean: “By default,”?
Context: ...t="350px" /> ### Custom Selector Icon By default the select uses a chevron-down icon a...

(BY_DEFAULT_COMMA)

---

176-176: Consider adding a comma after "By default".

According to the static analysis hint, adding a comma after "By default" would improve the sentence structure.

-By default the select will render the selected item's text value, but you can customize this by passing a `renderValue` function.
+By default, the select will render the selected item's text value, but you can customize this by passing a `renderValue` function.

Tools

LanguageTool

[uncategorized] ~176-~176: Did you mean: “By default,”?
Context: ...ht="350px" /> ### Custom Render Value By default the select will render the selected ite...

(BY_DEFAULT_COMMA)

---

250-250: Consider adding a comma to improve the sentence structure.

According to the static analysis hint, adding a comma would improve the sentence structure.

-You can render any component as the select value by using the `renderValue` property. In this example we are
+You can render any component as the select value by using the `renderValue` property. In this example, we are
 using the [Chip](/docs/components/chip) component to render the selected items.

Tools

LanguageTool

[typographical] ~250-~250: It appears that a comma is missing.
Context: ...ing the renderValue property. In this example we are using the [Chip](/docs/component...

(DURING_THAT_TIME_COMMA)

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3a3cd11 and bf7fe71.

Files selected for processing (2)

• apps/docs/content/components/select/open-state.ts (1 hunks)
• apps/docs/content/docs/components/select.mdx (9 hunks)

Files skipped from review due to trivial changes (1)

• apps/docs/content/components/select/open-state.ts

Additional context used

LanguageTool

apps/docs/content/docs/components/select.mdx

[uncategorized] ~122-~122: Did you mean: “By default,”?
Context: ...t="350px" /> ### Custom Selector Icon By default the select uses a chevron-down icon a...

(BY_DEFAULT_COMMA)

---

[uncategorized] ~176-~176: Did you mean: “By default,”?
Context: ...ht="350px" /> ### Custom Render Value By default the select will render the selected ite...

(BY_DEFAULT_COMMA)

---

[typographical] ~250-~250: It appears that a comma is missing.
Context: ...ing the renderValue property. In this example we are using the [Chip](/docs/component...

(DURING_THAT_TIME_COMMA)

Additional comments not posted (14)

apps/docs/content/docs/components/select.mdx (14)

48-48: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples.

---

57-57: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples.

---

63-63: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples.

---

73-73: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples.

---

80-80: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples.

---

84-84: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The different height for this example might be intentional to accommodate its specific content.

---

88-88: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The different height for this example might be intentional to accommodate its specific content.

---

92-92: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The consistent height with the previous example might be intentional to maintain visual coherence between related examples.

---

96-96: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The different height for this example might be intentional to accommodate its specific content.

---

102-102: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The consistent height with the previous example might be intentional to maintain visual coherence between related examples.

---

111-111: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The height is consistent with earlier examples and might be intentional to accommodate the content of this specific example.

---

119-119: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The consistent height with the previous example might be intentional to maintain visual coherence between related examples.

---

126-126: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The consistent height with the previous examples might be intentional to maintain visual coherence between related examples.

---

135-135: LGTM!

Setting a fixed previewHeight ensures consistent presentation across examples. The consistent height with the previous examples might be intentional to maintain visual coherence between related examples.

[macci001]

Hi @wingkwong,
This PR will required the use of the useAriaOverlay in Select. While this was implemented in the pr3467
Since the pr3467 was rolled back here in pr3759.
I think this PR needs to wait for the useAriaOverlay implementation to get in.
Can you please mark this as blocked on issue 3619 which tracks the above work?

Edit:
Also, the issue with docs you mentioned is fixed by adding an preview height to the demo code. The fix is added to the PR.
Reference video of docs containing the fix:

Screen.Recording.2024-09-16.at.12.18.00.PM.mov

[wingkwong]

Marked as on hold first. I think setting previewHeight is not correct. It shouldn't behave this way.