Conversation
|
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
|
|
👁 Visual Regression Report for PR #56500 Failed ❌
Important There are 3 diffs found in this PR: 🔄
|
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 significantly enhances the accessibility and user experience of the 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
|
size-limit report 📦
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new focusable prop to the Modal component to centralize focus management settings, deprecating older props like focusTriggerAfterClose and autoFocusButton. The changes are well-structured, including updates to hooks, interfaces, documentation, and tests. My review includes a critical fix for a logic bug in how focus settings are merged and a suggestion to improve type definitions for better maintainability.
| const mergedAutoFocusButton = React.useMemo(() => { | ||
| const base = focusable?.autoFocusButton || autoFocusButton; | ||
| return base || base === null ? base : 'ok'; | ||
| }, [autoFocusButton, focusable?.autoFocusButton]); |
There was a problem hiding this comment.
The logic for determining mergedAutoFocusButton has a bug and could be simplified. Using the logical OR operator (||) causes focusable.autoFocusButton: null to be ignored, incorrectly falling back to the legacy autoFocusButton prop. You should use the nullish coalescing operator (??) to correctly prioritize the focusable prop. The logic can also be made more readable.
| const mergedAutoFocusButton = React.useMemo(() => { | |
| const base = focusable?.autoFocusButton || autoFocusButton; | |
| return base || base === null ? base : 'ok'; | |
| }, [autoFocusButton, focusable?.autoFocusButton]); | |
| const mergedAutoFocusButton = React.useMemo(() => { | |
| const base = focusable?.autoFocusButton ?? autoFocusButton; | |
| return base === undefined ? 'ok' : base; | |
| }, [autoFocusButton, focusable]); |
| focusable?: FocusableConfig & { | ||
| autoFocusButton?: null | 'ok' | 'cancel'; | ||
| }; |
There was a problem hiding this comment.
For better code clarity and maintainability, consider defining a dedicated interface for the focusable prop in ModalFuncProps instead of using an inline intersection type. This makes the type easier to read and reuse.
You could define it like this earlier in the file:
export interface ModalFocusableConfig extends FocusableConfig {
autoFocusButton?: null | 'ok' | 'cancel';
}And then use it within ModalFuncProps:
// ...
focusable?: ModalFocusableConfig;
// ...
More templates
commit: |
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature #56500 +/- ##
===========================================
Coverage ? 100.00%
===========================================
Files ? 807
Lines ? 14870
Branches ? 3931
===========================================
Hits ? 14870
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
中文版模板 / Chinese template
🤔 This is a ...
🔗 Related Issues
💡 Background and Solution
📝 Change Log
focusable.trapto enable config focus lock in the Modal or not.focusable.trap以配置是否将焦点锁定在 Modal 内部。