introduce ChatCameraModal. ChatCameraModal factors out all the logic we use to send a multimedia message from CameraModal so now CameraModal can be more easily used for different use cases like taking a photo for your avatar
Details
- Reviewers
ashoat atul - Commits
- rCOMM0c35edd84354: [native] introduce ChatCameraModal component
Built the app on my phone, tested the camera ux and everything behaved as before also was still able to send multimedia messages with photos taken from the camera; no flow errors
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Just one serious issue
native/media/camera-modal.react.js | ||
---|---|---|
231 ↗ | (On Diff #25323) | Add a + here please. Make sure you always do this in React props |
244 ↗ | (On Diff #25323) | Why don't we use our ScreenParamList here? That will makes sure navigate calls are typed correctly |
854 ↗ | (On Diff #25323) | I think you're introducing a regression here, where multiple presses of the close button will lead to the screen that opened CameraModal being dismissed too. In the future, this is the type of the thing you should be able to notice before you put the diff up. When you see goBackOnce, you should ask yourself "why does this exist, and why isn't it in stock React Navigation"? If we implemented some custom functionality, there's almost certainly a good reason for that. You should not be removing that functionality without first understanding it. Options to address this:
|
native/media/camera-modal.react.js | ||
---|---|---|
854 ↗ | (On Diff #25323) | Had another idea to address this and wanted to run by you... Currently we can use goBackOnce through the navigation prop in ChatCameraModal since we are using AppNavigationProp (seems like all modals in our codebase use this). Would it be okay if we passed that navigation prop down to CameraModal and use that to replace the navigation we get from the React navigation useNavigation hook? We would then be able to call goBackOnce from within CameraModal |
native/media/camera-modal.react.js | ||
---|---|---|
854 ↗ | (On Diff #25323) | Good idea – that would work as long as the new CameraModals you're introducing for user and thread avatar selection are handled by the same React Navigation navigator! (I think you should probably be putting them in the same navigator) |
native/media/camera-modal.react.js | ||
---|---|---|
228 ↗ | (On Diff #25406) | I know it is better to have a RouteName type variable here like this example, and I did try to implement it, but I was spending a fair amount of time playing whack a mole with flow errors, and I feel like it would just be more productive if I did this for now, and came back to it after e2e camera/ens avatars are done |
native/media/camera-modal.react.js | ||
---|---|---|
228 ↗ | (On Diff #25406) | You can't do what's done there because Flow doesn't support generics on class declarations in the same way it does for function declarations. Options here:
If neither of those work, let me know and I'll try to patch locally and take a look. |
If neither of those work, let me know and I'll try to patch locally and take a look.
Unfortunately still getting flow errors with this:
When I tried AppNavigationProp<$Keys<OverlayParamList>> I got this error:
When I tried AppNavigationProp<mixed> I got this error:
Need to do something for atul, but I will keep investigating once I am done
native/media/camera-modal.react.js | ||
---|---|---|
228 ↗ | (On Diff #25406) | Never mind, tried both of those and they don't work. Going to look into it for a sec |
native/media/camera-modal.react.js | ||
---|---|---|
229–231 ↗ | (On Diff #25434) | To make this work, we needed it to be AppNavigationProp<'RouteOne'> | AppNavigationProp<'RouteTwo'> instead of AppNavigationProp<'RouteOne' | 'RouteTwo'>. To understand what I did here, first read through these:
Next, here's an explanation: We start from OverlayParamList, which is an object pointing from route names to params for those route names. We don't care about the params (the values), and only care about the route names (the keys), so we use $ObjMapi to map from the keys. We specify a function that maps from the route name K to AppNavigationProp<K>, so the result of the $ObjMapi is an object that maps from the route name to the AppNavigationProp for that route names. We then use $Values to get the union of all of the values in the object. |
Thanks for helping out here @ashoat! Definitely would have taken me a ton of time to figure that out. As I was working with the [User/Thread]AvatarCameraModal I had the type as AppNavigationProp<'ChatCameraModal'> | AppNavigationProp<'UserAvatarCameraModal'> for a placeholder, and after reading your explanation this helped connect the dots.