Page MenuHomePhabricator

[native] introduce ChatCameraModal component
ClosedPublic

Authored by ginsu on Apr 19 2023, 12:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 1:26 PM
Unknown Object (File)
Fri, Nov 22, 10:57 PM
Unknown Object (File)
Sun, Nov 3, 8:15 PM
Unknown Object (File)
Oct 28 2024, 7:06 PM
Unknown Object (File)
Oct 28 2024, 6:42 AM
Unknown Object (File)
Oct 28 2024, 6:42 AM
Unknown Object (File)
Oct 28 2024, 6:42 AM
Unknown Object (File)
Oct 28 2024, 6:42 AM
Subscribers

Details

Summary

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

Test Plan

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

ginsu added reviewers: ashoat, atul.
ginsu edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.EditedApr 19 2023, 6:29 AM

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:

  1. Introduce goBackOnce to ChatRouter (similar to how we do it in RootRouter and OverlayRouter), and introduce a new ProfileRouter with goBackOnce for ProfileNavigator.
  2. Find another way to make sure multiple goBacks aren't triggered, eg. by using some instance variable to track if it's already been pressed.
This revision now requires changes to proceed.Apr 19 2023, 6:29 AM
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

ashoat requested changes to this revision.Apr 19 2023, 4:55 PM
ashoat added inline comments.
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:

  1. Wrap the class declaration in a function declaration like we do with createTooltip, and then make the function declaration generic (more work than ideal and messy)
  2. Convert CameraModal to a function component (way too much work for now)
  3. (Best option) This doesn't actually really need to be generic, in the sense that there's nothing you need here specific to this particular route. Try replacing AppNavigationProp<'ChatCameraModal'> with one of the following:
    • AppNavigationProp<$Keys<OverlayParamList>>
    • AppNavigationProp<mixed>

If neither of those work, let me know and I'll try to patch locally and take a look.

This revision now requires changes to proceed.Apr 19 2023, 4:55 PM

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:

Screenshot 2023-04-19 at 8.32.15 PM.png (354×1 px, 98 KB)

When I tried AppNavigationProp<mixed> I got this error:

Screenshot 2023-04-19 at 8.30.23 PM.png (792×1 px, 243 KB)

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

ashoat added inline comments.
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:

  1. My GitHub issue on the Flow repo from 2018 about how to "map" a union of disjoint types. Eg. how to map from 'RouteOne' | 'RouteTwo' to AppNavigationProp<'RouteOne'> | AppNavigationProp<'RouteTwo'>.
  2. The docs for $ObjMap, which $ObjMapi is sort of based on.
  3. The docs for $ObjMapi.

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.

This revision is now accepted and ready to land.Apr 19 2023, 6:02 PM

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.

This revision was landed with ongoing or failed builds.Apr 19 2023, 6:27 PM
This revision was automatically updated to reflect the committed changes.