feat(dialog): implement dialog v2 #93
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change introduces a major refactor and platform abstraction for the dialog component system across multiple apps and the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Dialog
participant Trigger
participant Portal
participant Overlay
participant Content
participant Close
App->>Dialog: Render Dialog (Root)
Dialog->>Trigger: Render Trigger
Trigger->>Dialog: onPress → open dialog
Dialog->>Portal: Render Portal (if open)
Portal->>Overlay: Render Overlay (with animation)
Portal->>Content: Render Content (with animation)
Content->>Close: Render Close Button
Close->>Dialog: onPress → close dialog
Dialog->>Portal: Unmount Portal (if closed)
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🪛 Biome (1.9.4)packages/dialog/src/native/dialog-native.tsx[error] 73-73: This code is unreachable ... because this statement will throw an exception beforehand (lint/correctness/noUnreachable) packages/dialog/src/web/dialog-web.tsx[error] 73-73: This code is unreachable ... because this statement will throw an exception beforehand (lint/correctness/noUnreachable) 🔇 Additional comments (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (7)
packages/dialog/package.json (1)
42-47: Remove Duplicate@rn-primitives/portalDeclaration
@rn-primitives/portalis listed in bothdependencies(line 44) anddevDependencies(line 49). Keep it only underdependenciesto avoid redundancy.Also applies to: 48-50
packages/dialog/src/utils/contexts.ts (2)
4-4: Consider more explicit typing for the context.The context is initialized with
nullbut the type doesn't explicitly indicate nullability. Consider making this more explicit:-const RootContext = React.createContext<BaseDialogRootContext>(null); +const RootContext = React.createContext<BaseDialogRootContext | null>(null);This makes the nullable nature of the context more apparent to TypeScript and developers.
5-11: Improve error message specificity.The current error message is helpful but could be more specific about which components are considered compound components.
- throw new Error('Dialog compound components cannot be rendered outside the Dialog component'); + throw new Error('Dialog compound components (Close, Content, Description, Overlay, Portal, Title, Trigger) cannot be rendered outside the Dialog.Root component');This provides clearer guidance on which components require the context and what the proper parent should be.
packages/dialog/src/native/dialog-native.native.tsx (1)
35-35: Remove unnecessary Fragment wrapper.The Fragment is redundant as it only contains a single child element.
Apply this diff:
- <>{children}</> + {children}🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/nextjs-nativewind/src/components/ui/dialog.tsx (1)
19-41: Minor formatting issue: trailing spaceThere's a trailing space after "absolute" in the native styles.
- native: 'bg-black/50 dark:bg-black/80 flex justify-center items-center p-2 absolute ', + native: 'bg-black/50 dark:bg-black/80 flex justify-center items-center p-2 absolute',apps/nextjs-no-rn/src/components/ui/dialog.tsx (1)
18-40: Minor formatting issue: trailing spaceSame as in the other dialog implementation - there's a trailing space after "absolute" in the native styles.
- native: 'bg-black/50 dark:bg-black/80 flex justify-center items-center p-2 absolute ', + native: 'bg-black/50 dark:bg-black/80 flex justify-center items-center p-2 absolute',apps/expo-nativewind/components/ui/dialog.tsx (1)
74-83: Extract close button className for better readability.The close button has a complex inline className that would be more maintainable as a constant.
Consider extracting the className:
+const CLOSE_BUTTON_CLASS = 'absolute right-4 top-4 p-0.5 web:group rounded-sm opacity-70 web:ring-offset-background web:transition-opacity web:hover:opacity-100 web:focus:outline-none web:focus:ring-2 web:focus:ring-ring web:focus:ring-offset-2 web:disabled:pointer-events-none'; + function DialogContent({ className, children, native: { portalHost, ...nativeProp } = {}, ...props }: Omit<DialogPrimitive.ContentProps, 'native'> & { native?: DialogPrimitive.ContentProps['native'] & { portalHost?: string }; }) { const { open } = DialogPrimitive.useRootContext(); return ( <DialogPortal native={portalHost ? { hostName: portalHost } : undefined}> <DialogOverlay> <DialogPrimitive.Content asChild={Platform.OS === 'web'}> {/* DialogPrimitive.Content uses `nativeID` for accessibility, so it prevents the entering animation from working https://docs.swmansion.com/react-native-reanimated/docs/layout-animations/entering-exiting-animations/#remarks */} <View className={cn( Platform.select({ web: 'fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg', native: 'z-50 max-w-lg gap-4 border bg-background p-6 shadow-lg border-border rounded-lg', }), className )} native={mergeProps(CONTENT_NATIVE_PROPS, nativeProp)} {...props} > {children} <DialogPrimitive.Close - className={ - 'absolute right-4 top-4 p-0.5 web:group rounded-sm opacity-70 web:ring-offset-background web:transition-opacity web:hover:opacity-100 web:focus:outline-none web:focus:ring-2 web:focus:ring-ring web:focus:ring-offset-2 web:disabled:pointer-events-none' - } + className={CLOSE_BUTTON_CLASS} > <X size={Platform.OS === 'web' ? 16 : 18} className={cn('text-muted-foreground', open && 'text-accent-foreground')} /> </DialogPrimitive.Close> </View> </DialogPrimitive.Content> </DialogOverlay> </DialogPortal> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
apps/expo-nativewind/app/(components)/dialog.tsx(1 hunks)apps/expo-nativewind/components/ui/dialog.tsx(2 hunks)apps/nextjs-nativewind/src/app/page.tsx(3 hunks)apps/nextjs-nativewind/src/components/ui/dialog.tsx(2 hunks)apps/nextjs-no-rn/package.json(1 hunks)apps/nextjs-no-rn/src/app/page.tsx(3 hunks)apps/nextjs-no-rn/src/components/ui/alert-dialog.tsx(1 hunks)apps/nextjs-no-rn/src/components/ui/dialog.tsx(1 hunks)apps/vite-tanstack-router/package.json(1 hunks)apps/vite-tanstack-router/src/components/ui/dialog.tsx(1 hunks)apps/vite-tanstack-router/src/routes/index.tsx(3 hunks)packages/dialog/package.json(3 hunks)packages/dialog/src/base-types.ts(1 hunks)packages/dialog/src/dialog.tsx(0 hunks)packages/dialog/src/dialog.web.tsx(0 hunks)packages/dialog/src/index.ts(1 hunks)packages/dialog/src/native/dialog-native.native.tsx(1 hunks)packages/dialog/src/native/dialog-native.tsx(1 hunks)packages/dialog/src/native/index.ts(1 hunks)packages/dialog/src/native/types.ts(1 hunks)packages/dialog/src/types.ts(0 hunks)packages/dialog/src/universal/dialog.tsx(1 hunks)packages/dialog/src/universal/dialog.web.tsx(1 hunks)packages/dialog/src/universal/index.ts(1 hunks)packages/dialog/src/universal/types.ts(1 hunks)packages/dialog/src/utils/contexts.ts(1 hunks)packages/dialog/src/web/dialog-web.tsx(1 hunks)packages/dialog/src/web/dialog-web.web.tsx(1 hunks)packages/dialog/src/web/index.ts(1 hunks)packages/dialog/src/web/types.ts(1 hunks)packages/dialog/tsup.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/dialog/src/types.ts
- packages/dialog/src/dialog.web.tsx
- packages/dialog/src/dialog.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/dialog/src/utils/contexts.ts (10)
packages/dialog/src/base-types.ts (1)
BaseDialogRootContext(33-33)packages/dialog/src/native/dialog-native.tsx (1)
useRootContext(76-76)packages/dialog/src/native/index.ts (1)
useRootContext(10-10)packages/dialog/src/index.ts (1)
useRootContext(10-10)packages/dialog/src/universal/index.ts (1)
useRootContext(10-10)packages/dialog/src/universal/dialog.tsx (1)
useRootContext(58-58)packages/dialog/src/native/dialog-native.native.tsx (1)
useRootContext(196-196)packages/dialog/src/web/index.ts (1)
useRootContext(10-10)packages/dialog/src/web/dialog-web.tsx (1)
useRootContext(76-76)packages/dialog/src/web/dialog-web.web.tsx (1)
useRootContext(48-48)
apps/vite-tanstack-router/src/routes/index.tsx (3)
apps/vite-tanstack-router/src/components/ui/dialog.tsx (8)
Dialog(131-131)DialogTrigger(140-140)DialogContent(133-133)DialogHeader(136-136)DialogTitle(139-139)DialogDescription(134-134)DialogFooter(135-135)DialogClose(132-132)apps/vite-tanstack-router/src/components/ui/button.tsx (1)
Button(114-114)apps/vite-tanstack-router/src/components/ui/text.tsx (1)
Text(21-21)
apps/nextjs-nativewind/src/app/page.tsx (2)
apps/nextjs-nativewind/src/components/ui/dialog.tsx (1)
DialogClose(136-136)apps/nextjs-nativewind/src/components/ui/text.tsx (1)
Text(31-31)
packages/dialog/src/web/dialog-web.tsx (1)
packages/dialog/src/utils/contexts.ts (2)
useRootContext(15-15)RootContextReturnType(17-17)
packages/dialog/src/native/types.ts (2)
packages/dialog/src/native/index.ts (8)
RootProps(19-19)CloseProps(14-14)ContentProps(15-15)DescriptionProps(16-16)OverlayProps(17-17)PortalProps(18-18)TitleProps(20-20)TriggerProps(21-21)packages/dialog/src/base-types.ts (8)
BaseDialogRootProps(34-34)BaseDialogCloseProps(28-28)BaseDialogContentProps(29-29)BaseDialogDescriptionProps(30-30)BaseDialogOverlayProps(31-31)BaseDialogPortalProps(32-32)BaseDialogTitleProps(35-35)BaseDialogTriggerProps(36-36)
packages/dialog/src/universal/dialog.tsx (5)
packages/dialog/src/native/dialog-native.tsx (8)
Root(76-76)Content(76-76)Description(76-76)Overlay(76-76)Portal(76-76)Title(76-76)Trigger(76-76)Close(76-76)packages/dialog/src/native/index.ts (16)
Root(7-7)RootProps(19-19)Content(3-3)ContentProps(15-15)Description(4-4)DescriptionProps(16-16)Overlay(5-5)OverlayProps(17-17)Portal(6-6)PortalProps(18-18)Title(8-8)TitleProps(20-20)Trigger(9-9)TriggerProps(21-21)Close(2-2)CloseProps(14-14)packages/dialog/src/native/dialog-native.native.tsx (8)
Root(196-196)Content(196-196)Description(196-196)Overlay(196-196)Portal(196-196)Title(196-196)Trigger(196-196)Close(196-196)packages/dialog/src/universal/dialog.web.tsx (8)
Root(97-97)Content(97-97)Description(97-97)Overlay(97-97)Portal(97-97)Title(97-97)Trigger(97-97)Close(97-97)packages/dialog/src/universal/types.ts (8)
RootProps(103-103)ContentProps(99-99)DescriptionProps(100-100)OverlayProps(101-101)PortalProps(102-102)TitleProps(104-104)TriggerProps(105-105)CloseProps(98-98)
🪛 Biome (1.9.4)
packages/dialog/src/web/dialog-web.web.tsx
[error] 35-35: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
packages/dialog/src/web/dialog-web.tsx
[error] 73-73: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
packages/dialog/src/base-types.ts
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 21-21: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/dialog/src/native/dialog-native.tsx
[error] 73-73: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
packages/dialog/src/native/dialog-native.native.tsx
[error] 35-35: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (39)
packages/dialog/src/base-types.ts (1)
9-10: Well-structured base types with proper abstraction.The type definitions properly abstract the core dialog functionality while maintaining flexibility for platform-specific implementations. The use of
PickandOmitutilities ensures only relevant properties are exposed in each base type.Also applies to: 13-16, 23-26
apps/nextjs-no-rn/src/components/ui/alert-dialog.tsx (1)
6-6: Good import organization.Moving the
mergePropsutility import before the React import follows common import ordering conventions and improves code organization.apps/expo-nativewind/app/(components)/dialog.tsx (1)
33-35: Improved component abstraction with simplified usage.The simplified
DialogCloseusage without theasChildprop and Button wrapper indicates better abstraction in the dialog v2 implementation. The DialogClose component now handles button styling internally, making the API cleaner and more consistent.apps/nextjs-no-rn/package.json (1)
20-20: Necessary dependency addition for dialog v2 support.Adding the
@rn-primitives/dialogworkspace dependency is required to support the new modular dialog implementation. The dependency is properly placed and follows the existing workspace dependency pattern.apps/vite-tanstack-router/package.json (1)
21-21: LGTM! Dependency addition is correctly implemented.The workspace dependency for
@rn-primitives/dialogfollows the established pattern and is properly positioned alphabetically within the dependencies list.packages/dialog/tsup.config.ts (2)
4-15: LGTM! Build configuration properly supports modular architecture.The expanded entry points correctly reflect the new platform-specific structure with separate universal, native, and web implementations. The file extensions (.tsx, .native.tsx, .web.tsx) follow React Native conventions for platform-specific modules.
21-29: External dependencies properly configured for modular structure.The external array correctly includes the new internal module paths to prevent them from being bundled, which is essential for the platform-specific import resolution to work properly.
apps/nextjs-nativewind/src/app/page.tsx (3)
2-2: Minor: Import reordering.The
CollapsibleExampleimport was moved to an earlier position. This is a minor organizational change with no functional impact.
106-106: Dialog example now actively rendered.Good to see the
DialogExamplecomponent is now uncommented and actively displayed, allowing users to test the new dialog functionality.
246-248: DialogClose API simplified correctly.The removal of the
asChildprop and nestedButtonwrapper indicates thatDialogClosenow has built-in button styling and behavior. This simplification improves the API ergonomics while maintaining the same visual result.apps/nextjs-no-rn/src/app/page.tsx (3)
26-35: Dialog component imports properly added.The dialog component imports are complete and follow the established naming conventions. All necessary components are imported for the dialog example.
60-60: Dialog example correctly added to component list.The
DialogExampleis properly positioned in the component rendering sequence, allowing users to interact with and test the dialog functionality.
210-234: Dialog example implementation follows established patterns.The
DialogExampleimplementation is well-structured and consistent with dialog usage in other apps:
- Uses
DialogTriggerwithasChildpattern for the trigger button- Properly structured with
DialogHeader,DialogTitle, andDialogDescription- Uses the simplified
DialogCloseAPI without nested button wrapper- Includes appropriate styling classes for responsive design
This provides a good demonstration of the new dialog v2 API.
packages/dialog/src/universal/index.ts (2)
1-11: Modular Re-exports of Universal Components
The universal entrypoint cleanly re-exports all React primitives (Close,Content, etc.) from./dialog. This makes your public API surface explicit and maintainable.
13-22: Explicit Type Re-exports
Re-exporting the prop types from./typesaligns perfectly with the component exports above. Everything needed for consuming apps is exposed.packages/dialog/src/web/index.ts (2)
1-11: Web-Specific Component Exports
ExportingClose,Content,Description, etc. from./dialog-webcorrectly surfaces the web implementations.
13-22: Web-Specific Type Re-exports
Re-exporting the prop types from./typesmirrors the component exports, keeping the API consistent.packages/dialog/package.json (1)
3-27: Version Bump & Platform Entrypoints
Bumping to2.0.0-alpha.1and adding./native/./webunderexportsmatches the new build outputs and modular structure.packages/dialog/src/native/index.ts (1)
1-22: LGTM! Clean export organization.The native dialog index file follows standard practices for component library organization. The explicit named exports provide clear visibility into what's available from the native module.
packages/dialog/src/utils/contexts.ts (1)
13-17: LGTM! Clean type exports.The type alias and exports are well-organized and follow TypeScript best practices.
apps/vite-tanstack-router/src/routes/index.tsx (2)
21-30: LGTM! Comprehensive dialog imports.The dialog component imports are well-organized and include all necessary components for a complete dialog implementation.
64-64: Good integration of the new DialogExample.The DialogExample is properly integrated into the component showcase alongside other examples.
packages/dialog/src/index.ts (1)
1-22: Excellent improvement to explicit exports!The change from wildcard exports (
export * from) to explicit named exports is a significant improvement that provides:
- Better tree-shaking for bundlers
- Clearer API surface documentation
- More control over the public interface
- Better TypeScript intellisense
The explicit exports create a clean, predictable API surface for consumers of the dialog package.
packages/dialog/src/web/dialog-web.web.tsx (2)
17-39: Excellent implementation of the Root component with controllable state.The implementation properly handles both controlled and uncontrolled state patterns using the
useControllableStatehook, and provides the state via React context for child components. The integration with Radix UI primitives is well-structured.🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
41-46: Good use of primitive wrapping for cross-platform compatibility.The wrapping of Radix UI components with
withRNPrimitivesprovides a clean abstraction layer that enables consistent prop interfaces across platforms while maintaining the underlying Radix UI functionality.packages/dialog/src/native/types.ts (1)
1-49: Excellent type definition structure with clear platform separation.This type definition file demonstrates excellent TypeScript practices:
- Clear separation between native-only props and combined props
- Proper composition using intersection types
- Consistent naming conventions
- Comprehensive exports for both individual and combined types
The approach of extending base dialog props with platform-specific primitive props provides good type safety and flexibility for native implementations.
packages/dialog/src/native/dialog-native.tsx (1)
13-67: Good platform guard implementation pattern.The stub component pattern effectively prevents misuse of native components on unsupported platforms while providing clear developer feedback. This approach helps maintain platform boundaries in the dialog system.
packages/dialog/src/universal/dialog.tsx (1)
1-59: Well-structured universal dialog implementation!The implementation correctly forwards props to native components while ignoring web-specific props. The consistent pattern across all components and proper ref forwarding with type casting ensures TypeScript compatibility.
packages/dialog/src/native/dialog-native.native.tsx (3)
1-197: Excellent accessibility implementation!The dialog components provide comprehensive accessibility features including:
- Proper ARIA attributes (
aria-modal,aria-labelledby,aria-describedby)- Hardware back button handling for Android
- Accessibility escape gesture support
- Semantic roles for all interactive elements
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
117-160:⚠️ Potential issueFix missing dependency and property access error.
Two issues need to be addressed:
- Missing
onOpenChangedependency in useEffect- Incorrect boolean check on line 144
Apply this diff:
React.useEffect(() => { const backHandler = BackHandler.addEventListener('hardwareBackPress', () => { onOpenChange(false); return true; }); return () => { backHandler.remove(); }; - }, []); + }, [onOpenChange]);if (!forceMount) { - if (!value) { + if (!value.open) { return null; } }Likely an incorrect or invalid review comment.
94-115:⚠️ Potential issueFix variable reference and property access errors.
There are two critical issues in this component:
- Line 102 references an undefined variable
onAccessibilityEscape- Line 109 incorrectly checks
!valueinstead of!value.openApply this diff to fix both issues:
const => { - if (typeof 'function') { - onAccessibilityEscape(); + if (typeof 'function') { + onAccessibilityEscapeProp(); } onOpenChange(false); }, [onAccessibilityEscapeProp, onOpenChange]); if (!forceMount) { - if (!value) { + if (!value.open) { return null; } }Likely an incorrect or invalid review comment.
apps/vite-tanstack-router/src/components/ui/dialog.tsx (1)
1-142: Excellent platform-aware dialog implementation!The dialog components demonstrate:
- Thoughtful platform-specific styling with proper use of
Platform.select- Smooth animations using react-native-reanimated
- Proper prop merging for native animation configuration
- Clear documentation explaining the View wrapper necessity for animations
- Responsive design with Tailwind classes
apps/nextjs-nativewind/src/components/ui/dialog.tsx (2)
48-91: Well-structured platform abstraction with helpful documentationThe refactored
DialogContentcomponent effectively handles platform differences with:
- Clear separation of native animation props
- Helpful comment explaining the
nativeIDanimation conflict- Proper portal host configuration support
- Consistent platform-specific styling
The integration of the close button within the content view is clean and maintains proper styling across platforms.
104-123: Good simplification of title and description componentsThe refactoring from arrow functions with ref forwarding to plain function components is a good architectural decision. This simplification aligns with the component usage patterns and reduces unnecessary complexity.
packages/dialog/src/universal/dialog.web.tsx (2)
30-77: Consistent and well-structured web wrapper implementationsThe implementation follows a consistent pattern across all components:
- Proper handling of web/native prop separation
- Conditional wrapping with View/Text when styles are needed
- Clean destructuring to ignore native props on web platform
This approach provides good flexibility for styling while maintaining the Radix UI primitive behavior.
79-95: Good use of composition with Pressable wrappersThe
TriggerandClosecomponents effectively use theasChildpattern to wrap Radix primitives withPressable, providing:
- Proper button semantics on web with
as: 'button'- Consistent prop merging with
mergeProps- Clean separation of concerns
This pattern maintains accessibility while adding React Native compatibility.
apps/nextjs-no-rn/src/components/ui/dialog.tsx (1)
1-141: Excellent consistency across dialog implementationsThis implementation maintains excellent consistency with the
nextjs-nativewinddialog component, demonstrating:
- Shared animation and styling patterns
- Consistent platform abstraction approach
- Appropriate variations (lucide-react icons, Button component usage) for the app context
The code reuse and architectural alignment across apps shows good engineering practices.
apps/expo-nativewind/components/ui/dialog.tsx (2)
17-39: LGTM! Well-structured overlay component with platform-specific animations.The implementation properly handles platform differences and allows customization through prop merging while maintaining sensible animation defaults.
91-130: LGTM! Clean implementation of dialog sub-components.The header, footer, title, description, and close components are well-structured with appropriate platform-specific styling and consistent use of the design system through button variants and text contexts.
|
|
||
| type BaseDialogTitleProps = {}; | ||
|
|
||
| type BaseDialogDescriptionProps = {}; |
There was a problem hiding this comment.
Replace empty object type with a more specific type definition.
Replace the {} type with a more specific type definition to improve type safety.
-type BaseDialogDescriptionProps = {};
+type BaseDialogDescriptionProps = Record<string, never>;📝 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.
| type BaseDialogDescriptionProps = {}; | |
| type BaseDialogDescriptionProps = Record<string, never>; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🤖 Prompt for AI Agents
In packages/dialog/src/base-types.ts at line 21, replace the empty object type
`{}` in the BaseDialogDescriptionProps type alias with a more specific type
definition that accurately reflects the expected properties. Define explicit
properties or use an appropriate interface to improve type safety and clarity.
|
|
||
| type BaseDialogOverlayProps = Pick<DialogOverlayProps, 'forceMount'>; | ||
|
|
||
| type BaseDialogCloseProps = {}; |
There was a problem hiding this comment.
Replace empty object type with a more specific type definition.
Same issue as with BaseDialogTriggerProps - the {} type reduces type safety.
-type BaseDialogCloseProps = {};
+type BaseDialogCloseProps = Record<string, never>;📝 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.
| type BaseDialogCloseProps = {}; | |
| type BaseDialogCloseProps = Record<string, never>; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🤖 Prompt for AI Agents
In packages/dialog/src/base-types.ts at line 17, replace the empty object type
{} for BaseDialogCloseProps with a more specific type definition to improve type
safety, similar to the approach used for BaseDialogTriggerProps. Define explicit
properties or use a more precise type instead of an empty object.
|
|
||
| type BaseDialogRootProps = Omit<Prettify<DialogProps>, 'children'>; | ||
|
|
||
| type BaseDialogTriggerProps = {}; |
There was a problem hiding this comment.
Replace empty object type with a more specific type definition.
The {} type is overly permissive and means "any non-nullable value", which reduces type safety. Consider defining the actual shape of trigger props or using Record<string, never> for truly empty objects.
-type BaseDialogTriggerProps = {};
+type BaseDialogTriggerProps = Record<string, never>;Alternatively, if trigger props are expected to have specific properties, define them explicitly:
-type BaseDialogTriggerProps = {};
+type BaseDialogTriggerProps = {
+ asChild?: boolean;
+ // Add other expected trigger props
+};📝 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.
| type BaseDialogTriggerProps = {}; | |
| -type BaseDialogTriggerProps = {}; | |
| +type BaseDialogTriggerProps = Record<string, never>; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🤖 Prompt for AI Agents
In packages/dialog/src/base-types.ts at line 11, the type BaseDialogTriggerProps
is currently defined as an empty object type {}, which is too permissive and
reduces type safety. Replace the {} with a more specific type definition that
accurately represents the expected properties of trigger props. If no properties
are expected, use Record<string, never> to indicate an empty object type
explicitly. Otherwise, define the exact properties and their types that trigger
props should have.
|
|
||
| type BaseDialogCloseProps = {}; | ||
|
|
||
| type BaseDialogTitleProps = {}; |
There was a problem hiding this comment.
Replace empty object type with a more specific type definition.
The {} type should be replaced with a more specific type definition for better type safety.
-type BaseDialogTitleProps = {};
+type BaseDialogTitleProps = Record<string, never>;📝 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.
| type BaseDialogTitleProps = {}; | |
| type BaseDialogTitleProps = Record<string, never>; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🤖 Prompt for AI Agents
In packages/dialog/src/base-types.ts at line 19, replace the empty object type
`{}` in the BaseDialogTitleProps type alias with a more specific type definition
that accurately reflects the expected properties for better type safety.
Identify the actual properties needed for BaseDialogTitleProps and define them
explicitly instead of using an empty object type.
| import type { | ||
| PressablePropsUniversal, | ||
| TextPropsUniversal, | ||
| ViewPropsUniversal, | ||
| } from '@rn-primitives/core'; | ||
| import type { Prettify } from '@rn-primitives/types'; | ||
| import type { |
There was a problem hiding this comment.
Missing React Type Import for ReactNode
You're using React.ReactNode in RootProps but haven't imported React or ReactNode. This will error in TS. Please add a type import and switch to ReactNode:
+import type { ReactNode } from 'react';
type RootProps = Prettify<
BaseDialogRootProps & {
- children?: React.ReactNode;
+ children?: ReactNode;
}
>;Also applies to: 44-48
🤖 Prompt for AI Agents
In packages/dialog/src/universal/types.ts around lines 1 to 7 and also lines 44
to 48, the type ReactNode is used without importing it, causing TypeScript
errors. Add an import statement to import ReactNode as a type from 'react' at
the top of the file, then replace all instances of React.ReactNode with just
ReactNode to fix the issue.
| const useRootContext = () => { | ||
| throw new Error( | ||
| 'Cannot access the native useRootContext on the web. Please import from `@rn-primitives/dialog` or `@rn-primitives/dialog/web`' | ||
| ); | ||
| return {} as RootContextReturnType; | ||
| }; |
There was a problem hiding this comment.
Remove unreachable code after throw statement.
The static analysis tool correctly identifies that the return statement after the throw is unreachable and will never execute.
Apply this diff to remove the unreachable code:
const useRootContext = () => {
throw new Error(
'Cannot access the native useRootContext on the web. Please import from `@rn-primitives/dialog` or `@rn-primitives/dialog/web`'
);
- return {} as RootContextReturnType;
};📝 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.
| const useRootContext = () => { | |
| throw new Error( | |
| 'Cannot access the native useRootContext on the web. Please import from `@rn-primitives/dialog` or `@rn-primitives/dialog/web`' | |
| ); | |
| return {} as RootContextReturnType; | |
| }; | |
| const useRootContext = () => { | |
| throw new Error( | |
| 'Cannot access the native useRootContext on the web. Please import from `@rn-primitives/dialog` or `@rn-primitives/dialog/web`' | |
| ); | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🤖 Prompt for AI Agents
In packages/dialog/src/native/dialog-native.tsx around lines 69 to 74, remove
the return statement after the throw error because it is unreachable code and
will never execute. Simply delete the line containing "return {} as
RootContextReturnType;" to fix the issue.
Summary by CodeRabbit
New Features
Refactor
Chores
Bug Fixes