feat(iOS, ContainedModal): add ContainedModal and ContainedModalProvider related files on JS side#4171
feat(iOS, ContainedModal): add ContainedModal and ContainedModalProvider related files on JS side#4171sgaczol wants to merge 17 commits into
Conversation
such approach is temporary - implementing a custom shadow node will be handled in a separate pr
- ContainedModal.tsx - ContainedModal.types.ts - ContainedModalProvider.tsx - ContainedModalProvider.types.ts - index.tsx
There was a problem hiding this comment.
Pull request overview
Adds the JS/Fabric surface for a new Gamma iOS modal primitive (ContainedModal) and its hosting context (ContainedModalProvider) so it can be consumed from React code and registered for RN Codegen.
Changes:
- Added two Fabric codegen native component definitions for
RNSContainedModalandRNSContainedModalProvider(iOS-only). - Added JS components, props types, and a barrel export for the new contained-modal module.
- Registered the new component view names in the package.json codegen components map.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/fabric/gamma/modals/contained-modal/ContainedModalProviderNativeComponent.ts | Adds codegen binding for the provider native view (excluded on Android). |
| src/fabric/gamma/modals/contained-modal/ContainedModalNativeComponent.ts | Adds codegen binding for the modal native view (excluded on Android) with isOpen. |
| src/components/gamma/modals/contained-modal/index.ts | Exposes public exports for the new modal/provider module. |
| src/components/gamma/modals/contained-modal/ContainedModalProvider.types.ts | Defines provider props as ViewProps. |
| src/components/gamma/modals/contained-modal/ContainedModalProvider.tsx | JS wrapper around the provider native component. |
| src/components/gamma/modals/contained-modal/ContainedModal.types.ts | Defines modal props, including required isOpen. |
| src/components/gamma/modals/contained-modal/ContainedModal.tsx | JS wrapper around the modal native component + host styling. |
| package.json | Registers RNSContainedModal* component view class names for codegen. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import React from 'react'; | ||
| import ContainedModalProviderNativeComponent from '../../../../fabric/gamma/modals/contained-modal/ContainedModalProviderNativeComponent'; | ||
| import { ContainedModalProviderProps } from './ContainedModalProvider.types'; |
| "RNSContainedModal": { | ||
| "className": "RNSContainedModalComponentView" | ||
| }, | ||
| "RNSContainedModalProvider": { | ||
| "className": "RNSContainedModalProviderComponentView" | ||
| } |
| export default codegenNativeComponent<NativeProps>( | ||
| 'RNSContainedModalProvider', | ||
| { | ||
| excludedPlatforms: ['android'], | ||
| }, | ||
| ); |
| import React from 'react'; | ||
| import ContainedModalProviderHostNativeComponent from '../../../../fabric/gamma/modals/contained-modal/ContainedModalProviderHostNativeComponent'; | ||
| import type { ContainedModalProviderProps } from './ContainedModalProvider.types'; |
| "RNSContainedModalHost": { | ||
| "className": "RNSContainedModalHostComponentView" | ||
| }, |
There was a problem hiding this comment.
it will be handled in a separate PR
| "RNSContainedModalHostProvider": { | ||
| "className": "RNSContainedModalProviderHostComponentView" | ||
| } |
t0maboro
left a comment
There was a problem hiding this comment.
left a few comments, but overall looks good
| width: 0, | ||
| height: 0, |
There was a problem hiding this comment.
just leaving note: dimensions might not be needed to define here in the future, maybe we can handle the logic similarly as in FormSheet with hitTest override + dedicated touch handler: https://github.com/software-mansion/react-native-screens/blob/main/ios/gamma/modals/form-sheet/RNSFormSheetHostComponentView.mm#L343-L348 - sth to keep in mind in a further development
t0maboro
left a comment
There was a problem hiding this comment.
last remark regarding the naming, rest is looking good
| * | ||
| * @platform ios | ||
| */ | ||
| providerKey: string; |
There was a problem hiding this comment.
I don't like using providerKey on both sides. From a semantic perspective, using providerKey here feels a bit ambiguous. What do you think about being more explicit about roles of components by using containerId (or containerKey) on the Provider level, and targetContainerId/Key on the Modal level?
Having it defined this way should make the relationship much clearer. The Modal "points to" a specific destination.
|
|
||
| export interface NativeProps extends ViewProps { | ||
| isOpen?: CT.WithDefault<boolean, false>; | ||
| providerKey?: CT.WithDefault<string, ''>; |
There was a problem hiding this comment.
same request as for public API types
…inerId for the modal
|
Before merging, please ensure that this doesn't cause any runtime crashes, because native components haven't been defined yet. |
There was a problem hiding this comment.
I haven't reviewed the code, only looked at the scope of this PR and I'm seconding @t0maboro here. Let's avoid landing this PR until we have at least mock, but complete implementation of the native component.
I don't require the native code to be added in this particular PR, but let's see it in stacked PR & then merge both PRs together in quick succession. Let's not introduce intermediate states onto the main. The idea behind a commit / PR is that is should be always self-contained.
Description
This PR adds
ContainedModalandContainedModalProviderrelated files on JS side.Closes https://github.com/software-mansion/react-native-screens-labs/issues/1358
Changes
package.jsonBefore & after - visual documentation
N/A
Test plan
N/A
Checklist