[native] Send native picker media directly
ClosedPublic
Actions

Authored by bartek on Feb 3 2023, 5:03 AM.

Details

Reviewers

ashoat

Commits

rCOMM3ec0182976f9: [native] Send native picker media directly

Summary

Implements concept described in https://linear.app/comm/issue/ENG-2562#comment-42809236

The threadInfo is not available when keyboard is dismissed by the system (iOS issue) while picking albums in native picker interface.
This workaround passes the thread information along with the native picker flow to be available even when the keyboard is already dismissed

Depends on D6497

Test Plan

On iOS: Opened a thread, opened the native picker interface, picked a photo in three ways:

directly from media keyboard
through native picker interface (the Photos tab - default)
through native picker, but by picking a photo from album (Albums tab)

All of them work as expected

Diff Detail

Repository

rCOMM Comm

Branch

barthap/expo-image-picker

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Feb 3 2023, 5:03 AM

bartek held this revision as a draft.

Herald added subscribers: atul, tomek. · View Herald TranscriptFeb 3 2023, 5:03 AM

bartek retitled this revision from [native] Send native picker media instantly to [native] Send native picker media directly.Feb 3 2023, 5:04 AM

Remove unused type

bartek published this revision for review.Feb 3 2023, 5:12 AM

bartek added inline comments.

native/keyboard/keyboard-input-host.react.js
51 ↗	(On Diff #21940)	These styles are unused, I'm going to get rid of them in a separate diff to keep this one clean

Harbormaster completed remote builds in B16064: Diff 21938.Feb 3 2023, 5:22 AM

Harbormaster completed remote builds in B16066: Diff 21940.

ashoat accepted this revision.Feb 3 2023, 11:10 AM

ashoat added inline comments.

native/keyboard/keyboard-input-host.react.js
69–70 ↗	(On Diff #21940)	These should be `$ReadOnly`
74–75 ↗	(On Diff #21940)	I would probably reverse this and default to the `threadInfo` provided to the function – that way the caller has more control I honestly think we could probably get rid of `keyboardState.getMediaGalleryThread` here – it appears to me that `result.threadInfo` should always be set
native/media/media-gallery-keyboard.react.js
50 ↗	(On Diff #21940)	Can you move this prop above `// Redux state`? Otherwise it seems to be implying that `threadInfo` comes from Redux
652–671 ↗	(On Diff #21940)	I would prefer to be explicit about the props. Typical pattern we use is to define this at the top: type BaseProps = { +threadInfo: ?ThreadInfo, }; type Props = { ...BaseProps, // Redux state +dimensions: DimensionsInfo, +foreground: boolean, +colors: Colors, +styles: typeof unboundStyles, }; And then we can use `BaseProps` here

This revision is now accepted and ready to land.Feb 3 2023, 11:10 AM

bartek added inline comments.Feb 3 2023, 11:53 AM

native/keyboard/keyboard-input-host.react.js
74–75 ↗	(On Diff #21940)	Tested 2. in all scenarios from the test plan and it works so I'll go with this one. Switching threads when the keyboard was open also works as expected.
native/media/media-gallery-keyboard.react.js
652–671 ↗	(On Diff #21940)	Agree, your way is way better ;)

Applied suggestions from code review:

Introduced type BaseProps for MediaGalleryKeyboard
The onMediaGalleryItemSelected function now always receives threadInfo
Fully got rid of keyboardState.getMediaGalleryThread in favor of the new way of passing this value
Made some types read only