feat(Page): add ref prop for main element#12588
Conversation
WalkthroughThe Page component now accepts an optional ChangesPage main ref support
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/react-core/src/components/Page/Page.tsx`:
- Around line 100-101: Update the Page component’s render/destructuring logic to
explicitly extract mainRef from the props before collecting ...rest, then
continue applying mainRef to the main section ref. Ensure mainRef is excluded
from the outer div’s spread props while preserving all other rest properties.
- Around line 137-140: Update the Page component’s mainRef handling so the
constructor or field initializer only creates a stable internal fallback, while
rendering and lifecycle logic resolve this.props.mainRef on each update. Track
the effective ref identity and rebind mousedown/touch listeners whenever it
changes, ensuring listeners are removed from the previous element and attached
to the current one.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f836f8df-09a3-4b35-b8e4-f280c29d03bd
📒 Files selected for processing (1)
packages/react-core/src/components/Page/Page.tsx
| /** Reference for the main section of the page */ | ||
| mainRef?: React.RefObject<HTMLDivElement>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent mainRef from leaking onto the outer DOM element.
Because mainRef is not destructured in render, it remains in ...rest and is spread onto the page’s outer <div>, causing an invalid custom DOM prop/warning. Remove it from rest while using it as the main element’s ref.
🤖 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 `@packages/react-core/src/components/Page/Page.tsx` around lines 100 - 101,
Update the Page component’s render/destructuring logic to explicitly extract
mainRef from the props before collecting ...rest, then continue applying mainRef
to the main section ref. Ensure mainRef is excluded from the outer div’s spread
props while preserving all other rest properties.
| getVerticalBreakpoint, | ||
| mainRef: undefined | ||
| }; | ||
| mainRef = createRef<HTMLDivElement>(); | ||
| mainRef = this.props?.mainRef ? this.props.mainRef : createRef<HTMLDivElement>(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve mainRef from current props, not once at construction.
The field initializer permanently captures the initial prop. If a caller supplies or replaces mainRef after mount, the component continues using the old ref; its mousedown/touch listeners are also attached to the wrong element. Keep a stable internal fallback, resolve the current prop during rendering/lifecycle updates, and rebind listeners when the ref identity changes.
🤖 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 `@packages/react-core/src/components/Page/Page.tsx` around lines 137 - 140,
Update the Page component’s mainRef handling so the constructor or field
initializer only creates a stable internal fallback, while rendering and
lifecycle logic resolve this.props.mainRef on each update. Track the effective
ref identity and rebind mousedown/touch listeners whenever it changes, ensuring
listeners are removed from the previous element and attached to the current one.
1 participant
What: Closes #12428
Adds
mainRefproperty to allow a user to pass their own ref to Page's main element.Summary by CodeRabbit