fix(menu): merge content props through getFloatingProps (#806)#2024
fix(menu): merge content props through getFloatingProps (#806)#2024kotAPI wants to merge 1 commit into
Conversation
Pass menu content props into getFloatingProps for correct handler merge. Fixes #806
|
📝 WalkthroughWalkthroughIn ChangesMenu Floating Props Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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 improved or stayed the same. Great job! 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/Menu/fragments/MenuPrimitiveContent.tsx`:
- Around line 39-43: In MenuPrimitiveContentProps, expand the type definition to
accept standard HTML div attributes (such as data attributes, event handlers,
and style properties) in addition to the currently restricted children and
className. In the MenuPrimitiveContent component where getFloatingProps is
called, modify the style composition to merge floatingStyles with any custom
style from props rather than overwriting it, ensuring that custom styles are
preserved while floating positioning styles are applied. This allows consumers
to pass through additional attributes and maintain style precedence as intended.
🪄 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: 74bd67b6-2871-4538-a642-9828134c9c20
📒 Files selected for processing (1)
src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx
| {...getFloatingProps({ | ||
| ...props, | ||
| className, | ||
| style: floatingStyles | ||
| })} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx"
echo "=== Type definition and call site ==="
nl -ba "$FILE" | sed -n '1,90p'
echo
echo "=== Check whether props type includes HTML attributes ==="
rg -n "export type MenuPrimitiveContentProps|HTMLAttributes<HTMLDivElement>" "$FILE" -C2
echo
echo "=== Check current style merge order in getFloatingProps ==="
rg -n "getFloatingProps\\(|\\.\\.\\.props|style:\\s*floatingStyles" "$FILE" -C2Repository: rad-ui/ui
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx"
echo "=== File contents lines 1-90 ==="
cat -n "$FILE" | head -90
echo ""
echo "=== Type definition ==="
rg -n "export type MenuPrimitiveContentProps" "$FILE" -A5
echo ""
echo "=== getFloatingProps call site ==="
rg -n "getFloatingProps" "$FILE" -B3 -A5Repository: rad-ui/ui
Length of output: 3337
Consumer prop composition remains incomplete and style merging overwrites custom styles.
The style: floatingStyles assignment at line 42 deterministically overwrites any style from ...props. Additionally, MenuPrimitiveContentProps narrowly restricts consumers to children and className only, preventing them from passing standard HTML div attributes (data attributes, event handlers, etc.) that the ...props spread attempts but cannot accommodate due to the type constraint.
Suggested fix
-export type MenuPrimitiveContentProps = {
- children: React.ReactNode;
- className?: string;
-};
+export type MenuPrimitiveContentProps = React.HTMLAttributes<HTMLDivElement> & {
+ children: React.ReactNode;
+};
const MenuPrimitiveContent = forwardRef<HTMLDivElement, MenuPrimitiveContentProps>(
- ({ children, className, ...props }, propRef) => {
+ ({ children, className, style: styleProp, ...props }, propRef) => {
const context = useContext(MenuPrimitiveRootContext);
const mergedRef = Floater.useMergeRefs([
context?.refs.setFloating,
@@ -37,7 +37,7 @@ const MenuPrimitiveContent = forwardRef<HTMLDivElement, MenuPrimitiveContentPro
<div
ref={mergedRef}
{...getFloatingProps({
...props,
className,
- style: floatingStyles
+ style: { ...styleProp, ...floatingStyles }
})}🤖 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/Menu/fragments/MenuPrimitiveContent.tsx` around lines 39
- 43, In MenuPrimitiveContentProps, expand the type definition to accept
standard HTML div attributes (such as data attributes, event handlers, and style
properties) in addition to the currently restricted children and className. In
the MenuPrimitiveContent component where getFloatingProps is called, modify the
style composition to merge floatingStyles with any custom style from props
rather than overwriting it, ensuring that custom styles are preserved while
floating positioning styles are applied. This allows consumers to pass through
additional attributes and maintain style precedence as intended.
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
Test plan
Summary by CodeRabbit