fix(popover): correct position logic#4498
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces patches for three components from the NextUI library: Dropdown, Popover, and Select. The changes address issues related to the initial animation of the popover and the positioning of the dropdown arrow. The updates simplify the implementation of these components by removing unnecessary complexity, enhancing their placement logic, and adding new stories for testing various configurations. Changes
Sequence DiagramsequenceDiagram
participant User
participant Dropdown
participant Popover
User->>Dropdown: Trigger dropdown
Dropdown->>Popover: Determine placement
Popover-->>Dropdown: Apply correct transform origin
Dropdown->>User: Display with accurate positioning
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
🦋 Changeset detectedLatest commit: 386a421 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
| const placement = useMemo(() => { | ||
| // If ariaPlacement is null, popoverProps.style isn't set, | ||
| // so we return null to avoid an incorrect animation value. | ||
| if (!ariaPlacement) { | ||
| return null; | ||
| } | ||
|
|
||
| return getShouldUseAxisPlacement(ariaPlacement, placementProp) ? ariaPlacement : placementProp; | ||
| }, [ariaPlacement, placementProp]); |
There was a problem hiding this comment.
Incorrect initial placement caused animation to be applied in the wrong direction.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/select/stories/select.stories.tsx (1)
1498-1517: Consider clarifying the story name and adding tests
The newPopoverTopOrBottomstory effectively demonstrates how the popover behaves at the top and bottom, but the story name might benefit from "PopoverTopAndBottom" for clarity. Additionally, consider adding an automated test or visual regression check to confirm that the popover is positioned correctly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.changeset/lazy-rice-wash.md(1 hunks)packages/components/dropdown/package.json(0 hunks)packages/components/dropdown/src/use-dropdown.ts(3 hunks)packages/components/dropdown/stories/dropdown.stories.tsx(1 hunks)packages/components/popover/src/free-solo-popover.tsx(1 hunks)packages/components/popover/src/popover-content.tsx(2 hunks)packages/components/popover/src/use-popover.ts(6 hunks)packages/components/select/stories/select.stories.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/dropdown/package.json
✅ Files skipped from review due to trivial changes (1)
- .changeset/lazy-rice-wash.md
🔇 Additional comments (13)
packages/components/popover/src/free-solo-popover.tsx (1)
61-61: Confirm "center" mapping.
Whenplacementis"center", the transform origin is forced to"top". Verify if this is indeed the intended behavior for centrally placed popovers or if you need to differentiate between horizontal and vertical centering.packages/components/dropdown/src/use-dropdown.ts (3)
12-12: Import usage confirmed.
The newly importedariaShouldCloseOnInteractOutsideseems correctly integrated when constructing the popover logic.
92-92: Validate default placement.
You changed the defaultplacementto"bottom". Ensure this aligns with design requirements and doesn’t break existing UI expectations.
149-149: No apparent issues with merged popover props.
The updatedgetPopoverPropsmerges theplacementandshouldCloseOnInteractOutside. Everything looks consistent.packages/components/popover/src/use-popover.ts (6)
11-11: New utility import.
getShouldUseAxisPlacementis newly imported. Confirm it is used as intended for the axis-based placement logic.
156-160: RenamingplacementtoariaPlacement.
Destructuring theplacementasariaPlacementis clear. Just confirm references to the originalplacementvariable are now updated.
182-191: Returnnullplacement carefully.
WhenariaPlacementisnull, you returnnullforplacement. Ensure dependent logic can handle anullplacement gracefully without runtime errors.
224-224: Consolidatedata-placementlogic.
Setting"data-placement"helps style or test arrow positioning. This appears valid.
238-241: Ensure re-render triggers.
IncludingplacementandariaPlacementin the dependency array is prudent. This ensures consistent arrow placement updates.
325-325: Expose final placement.
Referencing the computedplacementhere is good. Confirm that external code can handle anullvalue if that arises.packages/components/dropdown/stories/dropdown.stories.tsx (1)
839-862: Great demonstration of all placements.
The newPlacementsstory thoroughly showcases top, bottom, left, and right placements (including-start/-end) for testing in various layouts.packages/components/popover/src/popover-content.tsx (2)
83-85: Validate fallback placement logic
Whenplacementis"center", the code uses"top"as a fallback for transform origins. Verify that"center"is valid or necessary, and confirm that the fallback to"top"is the desired outcome across all popover usage scenarios.
96-96: Approved usage of style prop
Applying the computedstyleprop in your Framer Motion component simplifies the code and maintains readability. This is a well-structured approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/autocomplete/stories/autocomplete.stories.tsx (1)
1190-1209: Consider adding a descriptive story title or a short doc string.
Currently named "PopoverTopOrBottom," it might be more intuitive to convey that this story illustrates how the autocomplete’s popover appears when placed at the top or in the middle. A short doc string or a more descriptive name can help future maintainers and contributors quickly grasp the scenario.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/lazy-rice-wash.md(1 hunks)packages/components/autocomplete/stories/autocomplete.stories.tsx(1 hunks)packages/components/select/stories/select.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/lazy-rice-wash.md
- packages/components/select/stories/select.stories.tsx
ryo-manba
changed the title
3 participants
Closes #4466
📝 Description
This PR fixes the initial position logic for the Popover.
Previously, the popover could animate in the wrong direction or fail to show the DropDown arrow.
⛳️ Current behavior (updates)
Autocomplete and Select animations are also incorrect.
🚀 New behavior
dropdown-arrow-placement.mov
Arrow is now applied correctly. However, the arrow’s position is slightly off for
left-startorright-endplacements, which will be addressed separately:See: https://github.com/nextui-org/nextui/blob/canary/packages/core/theme/src/components/popover.ts#L58-L61
select-popover.mov
autocomplete-popover.mov
I also fixed the issue causing the Select and Autocomplete animation to be misaligned.
💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
Related PRs
Summary by CodeRabbit
Bug Fixes
New Features
Refactor