Conversation
mmazzarolo
left a comment
mmazzarolo
left a comment
There was a problem hiding this comment.
Thanks for this PR! 🙌
Code looks perfect, but I'd like to suggest a different API.
| bottom: 0, | ||
| left: 0, | ||
| right: 0, | ||
| ...StyleSheet.absoluteFillObject, |
|
|
||
| if (!coverScreen && visible) { | ||
| return ( | ||
| <View pointerEvents="box-none" style={StyleSheet.absoluteFill}> |
There was a problem hiding this comment.
I know, in the other thread I suggested to just provide a coverScreen prop like we're doing in react-native-modal. Still, I think an even better approach for the maintainability of the project would be to instead allow providing a CustomModalComponent props that we'll just swap with ReactNativeModal below, if provided.
Why? Because with the current approach in the future people will also require something like coverScreenWrapperProps to inject here, and so on, making the surface area of this API even bigger.
The drawback of the CustomModalComponent approach is that users will need to maintain the wrapper (so line 208 to 210 in their app — but I'm 100% OK with it given that it's not something that I want to/can maintain within react-native-dialog :/
Hope it makes sense.
| } | ||
|
|
||
| return ( | ||
| <ReactNativeModal |
There was a problem hiding this comment.
Basically, following what I'm saying above, you can replace ReactNativeModal here with ModalComponent, which is a variable defined like this:
const ModalComponent = props.CustomModalComponent || ReactNativeModal;There was a problem hiding this comment.
Gotcha. Can do, I'll update the PR shortly.
2 participants
Fixes: #120
Overview
Test Plan