[native] Add function to open native picker
ClosedPublic
Actions

Authored by bartek on Feb 1 2023, 8:54 AM.

Details

Reviewers

ashoat
atul
inka
kamil

Commits

rCOMM4cda52ed4e81: [native] Add function to open native picker

Summary

Created a function that opens the native image picker interface using the expo-image-picker library and then processes the result to be compatible with our MediaLibrarySelection type. Finally, it updates the component state to make the selection prepended to the MediaGalleryKeyboard and queued to send.

Useful docs: image picker options, returned data.

Depends on D6493, D6494

Test Plan

Created a test button that triggers this function and ensured (both platforms) that:

native picker opens
multiple results can be picked
selected media are prepended to the gallery and enqueued to send

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Feb 1 2023, 8:54 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptFeb 1 2023, 8:54 AM

Harbormaster completed remote builds in B15940: Diff 21763.Feb 1 2023, 9:11 AM

bartek added a child revision: D6497: [native] Add native picker button to MediaGalleryKeyboard.Feb 1 2023, 9:51 AM

bartek edited the test plan for this revision. (Show Details)Feb 1 2023, 10:10 AM

bartek attached a referenced file: F355781: simulator-screen-recording-iphone-14-pro-2023-02-01-at-190533_sbSuHNEi.mp4. (Show Details)

bartek published this revision for review.Feb 1 2023, 10:15 AM

Demo looks great! nits and question inline

native/media/media-gallery-keyboard.react.js
302 ↗	(On Diff #21763)	Tiny nit but we usually do `ID` instead of `Id`
304 ↗	(On Diff #21763)	Thought it was a requirement of the filesystem (or the ones I'm familiar with) that every file must have a name? My assumption might be incorrect or not apply to all platforms?
336 ↗	(On Diff #21763)	Tiny nit but we'd usually style this as `selectionURIs`

bartek added inline comments.Feb 1 2023, 10:55 AM

native/media/media-gallery-keyboard.react.js
304 ↗	(On Diff #21763)	Replied here: https://phab.comm.dev/D6493#194506 We're doing our best effort here, and finally falling back to an empty string which is then handled by the media processing code.

atul added inline comments.Feb 1 2023, 10:58 AM

native/media/media-gallery-keyboard.react.js
304 ↗	(On Diff #21763)	Thanks for writing that up, looks like just two variable naming nits and this should be good to land

Passing back to you with question. If this is handled in a separate / later diff with a patch or something, feel free to let me know and I can accept

native/media/media-gallery-keyboard.react.js
316 ↗	(On Diff #21763)	I think it was possible for this value to be falsey on Android? `MediaLibrarySelection` expects it to be set, so we could be introducing some issues here. Context here

This revision now requires changes to proceed.Feb 1 2023, 11:01 AM

bartek added inline comments.Feb 1 2023, 11:08 AM

native/media/media-gallery-keyboard.react.js
316 ↗	(On Diff #21763)	That's right, it's optional and I should fix the types here. However, it is handled correctly, because when you `git grep mediaNativeID`, you can see that it is null-checked everywhere. I quickly set it to optional in the `type MediaLibrarySelection` definition and flow still doesn't complain.

ashoat added inline comments.Feb 1 2023, 11:16 AM

native/media/media-gallery-keyboard.react.js
316 ↗	(On Diff #21763)	Awesome!! Okay, so the only thing to do here it is to fix the types

Made mediaNativeID type optional
Applied suggestions regarding naming conventions

Harbormaster completed remote builds in B15989: Diff 21830.Feb 2 2023, 12:48 AM

Question to you, feel free to re-request review if this will be handled in a different place

native/media/media-gallery-keyboard.react.js
344 ↗	(On Diff #21830)	The plan is now to call `inputState.sendMultimediaMessage` from here directly, right? Is that mean to be handled in this diff or in a later one?