-
-
Notifications
You must be signed in to change notification settings - Fork 206
feat: support focusTrap #542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
演进概览本变更引入了一个新的 变更详情
预计代码审查工作量🎯 2 (简单) | ⏱️ ~10 分钟 可能相关的 PR
兔子之歌
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @zombieJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Dialog component by introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a focusTrap prop to allow disabling the focus lock feature in the dialog. The implementation is sound, and a corresponding test case has been added to verify the new behavior. I have one suggestion to improve the code's readability.
| const mergedRef = useComposeRef(holderRef, panelRef, internalRef); | ||
|
|
||
| useLockFocus(visible && isFixedPos, () => internalRef.current); | ||
| useLockFocus(visible && isFixedPos && focusTrap !== false, () => internalRef.current); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better readability and to use a more modern JavaScript/TypeScript feature, you could use the nullish coalescing operator (??). The expression focusTrap ?? true is equivalent to focusTrap !== false for boolean and undefined values, and it more clearly conveys the intent of defaulting to true when focusTrap is not provided. Note that the parentheses are important due to operator precedence.
| useLockFocus(visible && isFixedPos && focusTrap !== false, () => internalRef.current); | |
| useLockFocus(visible && isFixedPos && (focusTrap ?? true), () => internalRef.current); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #542 +/- ##
=======================================
Coverage 98.42% 98.42%
=======================================
Files 8 8
Lines 191 191
Branches 68 68
=======================================
Hits 188 188
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/IDialogPropTypes.ts (1)
61-61: 建议添加 JSDoc 注释说明属性用途。新增的
focusTrap属性缺少文档说明。建议参考同文件中focusTriggerAfterClose的注释风格,添加 JSDoc 注释说明该属性的作用、默认值和使用场景,以提升 API 的可发现性和开发者体验。🔎 建议的文档补充
+ // Whether to trap focus inside the dialog when it's open and fixed position + // Default: true focusTrap?: boolean;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Dialog/Content/Panel.tsxsrc/IDialogPropTypes.tstests/focus.spec.tsx
🔇 Additional comments (3)
tests/focus.spec.tsx (1)
68-84: 测试用例实现正确!新增的测试用例正确验证了
focusTrap={false}的行为:即使在position: fixed的情况下,焦点锁定也应该被禁用。测试结构遵循了现有测试的模式,实现清晰准确。src/Dialog/Content/Panel.tsx (2)
49-49: 属性解构实现正确!
focusTrap属性已正确添加到解构列表中,并且通过PanelProps extends IDialogPropTypes自动继承了类型定义。
57-57: 焦点锁定逻辑实现优秀!使用
focusTrap !== false模式是一个很好的实现选择,它提供了合理的默认行为:
- 未指定
focusTrap时默认启用焦点锁定(向后兼容)- 显式设置
focusTrap={false}时禁用焦点锁定- 显式设置
focusTrap={true}时启用焦点锁定这种实现方式保证了向后兼容性,现有未使用
focusTrap属性的代码将继续按原有方式工作。
|
手动 +1 🙌 |
Summary by CodeRabbit
发布说明
新功能
测试
✏️ Tip: You can customize this high-level summary in your review settings.