feat(Page): add ref prop for main element - #12588
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Page component now accepts an optional ChangesPage main ref support
Estimated code review effort: 2 (Simple) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
| 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.
2 participants
What: Closes #12428
Adds
mainRefproperty to allow a user to pass their own ref to Page's main element.Summary by CodeRabbit