fix(floating-ui): merge props through getReferenceProps (#806) - #1999
fix(floating-ui): merge props through getReferenceProps (#806)#1999kotAPI wants to merge 1 commit into
Conversation
|
📝 WalkthroughWalkthroughRefactors prop-getter functions ( ChangesDialog and Menu Prop-Getter Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage decreased for at least one metric. Please add or update tests to improve coverage. Run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/primitives/Dialog/fragments/DialogPrimitiveAction.tsx`:
- Around line 18-21: The onClick handler in the getItemProps call for
DialogPrimitiveAction is overwriting the consumer's onClick prop instead of
composing them together. Modify the onClick assignment to first call the
incoming props.onClick (if provided) and then call handleOpenChange(false),
following the same composition pattern used in the Cancel component. This
ensures that user-provided action callbacks execute before the dialog closes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcd4ab83-352d-41aa-833c-56deb5175d91
📒 Files selected for processing (5)
src/core/primitives/Dialog/context/DialogPrimitiveContext.tsxsrc/core/primitives/Dialog/fragments/DialogPrimitiveAction.tsxsrc/core/primitives/Dialog/fragments/DialogPrimitiveCancel.tsxsrc/core/primitives/Dialog/fragments/DialogPrimitiveTrigger.tsxsrc/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx
| {...getItemProps({ | ||
| ...props, | ||
| onClick: () => handleOpenChange(false) | ||
| })} |
There was a problem hiding this comment.
Preserve consumer onClick when composing Action handlers.
Line 20 overwrites incoming onClick, so forwarded action callbacks won’t run. Compose user onClick before handleOpenChange(false) (same pattern as Cancel).
Suggested fix
export type DialogPrimitiveActionProps = {
children: React.ReactNode;
className?: string;
asChild?: boolean;
+ onClick?: React.MouseEventHandler<HTMLButtonElement>;
}
-const DialogPrimitiveAction = forwardRef<HTMLButtonElement, DialogPrimitiveActionProps>(({ children, asChild, ...props }, ref) => {
+const DialogPrimitiveAction = forwardRef<HTMLButtonElement, DialogPrimitiveActionProps>(({ children, asChild, onClick, ...props }, ref) => {
const { handleOpenChange, getItemProps } = useContext(DialogPrimitiveContext);
return (
<ButtonPrimitive
ref={ref}
asChild={asChild}
{...getItemProps({
...props,
- onClick: () => handleOpenChange(false)
+ onClick: (event: React.MouseEvent<HTMLButtonElement>) => {
+ onClick?.(event);
+ handleOpenChange(false);
+ }
})}
>
{children}
</ButtonPrimitive>
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {...getItemProps({ | |
| ...props, | |
| onClick: () => handleOpenChange(false) | |
| })} | |
| export type DialogPrimitiveActionProps = { | |
| children: React.ReactNode; | |
| className?: string; | |
| asChild?: boolean; | |
| onClick?: React.MouseEventHandler<HTMLButtonElement>; | |
| } | |
| const DialogPrimitiveAction = forwardRef<HTMLButtonElement, DialogPrimitiveActionProps>(({ children, asChild, onClick, ...props }, ref) => { | |
| const { handleOpenChange, getItemProps } = useContext(DialogPrimitiveContext); | |
| return ( | |
| <ButtonPrimitive | |
| ref={ref} | |
| asChild={asChild} | |
| {...getItemProps({ | |
| ...props, | |
| onClick: (event: React.MouseEvent<HTMLButtonElement>) => { | |
| onClick?.(event); | |
| handleOpenChange(false); | |
| } | |
| })} | |
| > | |
| {children} | |
| </ButtonPrimitive> | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Dialog/fragments/DialogPrimitiveAction.tsx` around lines
18 - 21, The onClick handler in the getItemProps call for DialogPrimitiveAction
is overwriting the consumer's onClick prop instead of composing them together.
Modify the onClick assignment to first call the incoming props.onClick (if
provided) and then call handleOpenChange(false), following the same composition
pattern used in the Cancel component. This ensures that user-provided action
callbacks execute before the dialog closes.
Code reviewLGTM. Matches project patterns for portal Theme refs, Floating UI prop merge, controlled-switch/lazy-mount/RTL tests, or focused bug fixes. No changes requested. |
1 participant
Summary
Fixes #806, relates to #1099
Test plan
Summary by CodeRabbit