feat: add closeOnBackPress prop to Dialog #112
Conversation
|
@fxamauri is attempting to deploy a commit to the Ronin Technologies Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds a new optional Changes
Sequence DiagramsequenceDiagram
actor User
participant Dialog Root
participant Dialog Context
participant Dialog Content
participant Hardware
User->>Dialog Root: Define closeOnBackPress prop
Dialog Root->>Dialog Context: Provide closeOnBackPress value
Dialog Content->>Dialog Context: Read closeOnBackPress
Hardware->>Dialog Content: Back button pressed
alt closeOnBackPress === true
Dialog Content->>Dialog Content: Call onOpenChange(false)
Dialog Content->>User: Dialog closes
else closeOnBackPress === false
Dialog Content->>User: Dialog remains open
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/docs/src/content/docs/dialog.mdx(4 hunks)packages/dialog/src/dialog.tsx(3 hunks)packages/dialog/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/dialog/src/types.ts (4)
packages/popover/src/popover.tsx (3)
IRootContext(22-30)backHandler(211-223)onPress(145-165)packages/alert-dialog/src/alert-dialog.web.tsx (2)
open(25-39)props(108-131)packages/dialog/src/dialog.web.tsx (2)
open(25-39)props(93-100)packages/context-menu/src/context-menu.web.tsx (1)
close(77-79)
packages/dialog/src/dialog.tsx (1)
packages/popover/src/popover.tsx (1)
backHandler(211-223)
apps/docs/src/content/docs/dialog.mdx (1)
packages/dialog/src/dialog.web.tsx (4)
props(175-182)props(93-100)open(25-39)props(106-132)
🔇 Additional comments (4)
packages/dialog/src/types.ts (1)
14-18: LGTM! Type definitions are consistent and well-documented.The
closeOnBackPressprop is correctly defined as an optional boolean in bothRootContextandRootProps, with appropriate platform annotations. This aligns well with the implementation pattern.Also applies to: 25-29
packages/dialog/src/dialog.tsx (2)
28-37: LGTM! Root component properly handles the new prop.The
closeOnBackPressprop is correctly implemented with a sensible default value oftruethat maintains backward compatibility. The prop is properly forwarded through the context to descendant components.Also applies to: 48-59
148-151: Verify the intended back press event consumption behavior.The back handler always returns
true(consuming the event) regardless of thecloseOnBackPressvalue. This means whencloseOnBackPress={false}, the back button is still intercepted but the dialog stays open—users cannot use back navigation while the dialog is visible.If the intent is to allow back press propagation (e.g., to close the app or navigate) when
closeOnBackPress={false}, consider returning the value ofcloseOnBackPressinstead:const backHandler = BackHandler.addEventListener('hardwareBackPress', () => { if (closeOnBackPress) { onOpenChange(false); } - return true; + return closeOnBackPress; });Otherwise, the current behavior is correct for a modal dialog that should prevent back navigation.
apps/docs/src/content/docs/dialog.mdx (1)
140-146: LGTM! Documentation accurately reflects the new prop.The
closeOnBackPressprop is properly documented in the Root component's props table with the correct platform annotation and optional designation.
| React.useEffect(() => { | ||
| const backHandler = BackHandler.addEventListener('hardwareBackPress', () => { | ||
| onOpenChange(false); | ||
| if (closeOnBackPress) { | ||
| onOpenChange(false); | ||
| } | ||
| return true; | ||
| }); | ||
|
|
||
| return () => { | ||
| backHandler.remove(); | ||
| }; | ||
| }, []); | ||
| }, [closeOnBackPress]); |
There was a problem hiding this comment.
Add onOpenChange to the effect dependency array.
The useEffect hook calls onOpenChange inside the back handler but doesn't include it in the dependency array. This can lead to stale closure bugs where an outdated version of onOpenChange is invoked.
Apply this diff:
React.useEffect(() => {
const backHandler = BackHandler.addEventListener('hardwareBackPress', () => {
if (closeOnBackPress) {
onOpenChange(false);
}
return true;
});
return () => {
backHandler.remove();
};
- }, [closeOnBackPress]);
+ }, [closeOnBackPress, onOpenChange]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| React.useEffect(() => { | |
| const backHandler = BackHandler.addEventListener('hardwareBackPress', () => { | |
| onOpenChange(false); | |
| if (closeOnBackPress) { | |
| onOpenChange(false); | |
| } | |
| return true; | |
| }); | |
| return () => { | |
| backHandler.remove(); | |
| }; | |
| }, []); | |
| }, [closeOnBackPress]); | |
| React.useEffect(() => { | |
| const backHandler = BackHandler.addEventListener('hardwareBackPress', () => { | |
| if (closeOnBackPress) { | |
| onOpenChange(false); | |
| } | |
| return true; | |
| }); | |
| return () => { | |
| backHandler.remove(); | |
| }; | |
| }, [closeOnBackPress, onOpenChange]); |
🤖 Prompt for AI Agents
In packages/dialog/src/dialog.tsx around lines 146 to 157, the useEffect
registers a hardwareBackPress handler that calls onOpenChange but onOpenChange
is missing from the dependency array; update the effect dependencies to include
onOpenChange (i.e., change the dependency array from [closeOnBackPress] to
[closeOnBackPress, onOpenChange]) so the latest callback is used and
stale-closure bugs are avoided.
1 participant
Description
This PR adds a new
closeOnBackPressprop to theDialogcomponent, allowing to control how the physical BackPress button behaves on React Native.Motivation
Previously, the
Dialogwould always close automatically when the Android back button was pressed. In some scenarios, it's necessary to keep the dialog open or handle the back press action more granularly.Summary by CodeRabbit
Release Notes
New Features
closeOnBackPressprop to the Dialog Root component (native platforms only), allowing control over whether the dialog closes when pressing the hardware back button. Enabled by default.Documentation
closeOnBackPressprop.