[native] Move rest of Search views to `ChatThreadListSearch`
ClosedPublic
Actions

Authored by atul on Sep 14 2023, 1:24 PM.

Details

Reviewers

ginsu
tomek
rohan

Commits

rCOMMe9e030d6493b: [native] Move rest of Search views to `ChatThreadListSearch`

Summary

Okay so after incrementally recreating the changes from my initial attempt at moving search-related code from ChatThreadList to ChatThreadListSearch it seems like I found the issue. It seems that when I move the <View style={styles.searchContainer}> from ChatThreadList to ChatThreadListSearch a bunch of gesture handlers and onPress fail to fire? I have no idea why, probably due to my lack of experience using Reanimated, but I'll keep that View in the outer ChatThreadList for now and return a React.Fragment in ChatThreadListSearch.

Depends on D9205

Test Plan

Search experience continues to look and work as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Sep 14 2023, 1:24 PM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 14 2023, 1:24 PM

atul published this revision for review.Sep 14 2023, 1:25 PM

atul added inline comments.

native/chat/chat-thread-list.react.js
243 ↗	(On Diff #31151)	For some reason if I move this `<View>` "down" to `ChatThreadListSearch` things start to break? Specifically gesture handlers and `onPress` don't fire as expected. CC @ashoat, @tomek for thoughts?

atul added a child revision: D9207: [native] Move Reanimated code from `ChatThreadList` to `ChatThreadListSearch`.Sep 14 2023, 1:31 PM

Harbormaster completed remote builds in B22640: Diff 31151.Sep 14 2023, 1:43 PM

ashoat added inline comments.Sep 15 2023, 7:17 PM

native/chat/chat-thread-list.react.js
243 ↗	(On Diff #31151)	I think this is happening due to the quirks of `TouchableWithoutFeedback`. I think it actually “clone”s the JSX it’s passed with some additional props being set. It needs the component that it’s passed to be a `View`, or else to forward props to a view. You could probably solve this by making `ChatThreadListSearch` accept `…ViewProps` and forward it to the `View` that is being rendered

atul added inline comments.Sep 17 2023, 4:19 PM

native/chat/chat-thread-list.react.js
243 ↗	(On Diff #31151)	Ah gotcha thanks for explaining, that makes a lot more sense.

tomek accepted this revision.Sep 18 2023, 3:46 AM

tomek added inline comments.

native/chat/chat-thread-list.react.js
34 ↗	(On Diff #31151)	We're importing `chat-thread-list-search` here which imports `SearchStatus` type from this file, so we're creating a loop. Not sure if this kind of loop causes any issues though, because it is only types import.

This revision is now accepted and ready to land.Sep 18 2023, 3:46 AM

atul added inline comments.Sep 18 2023, 9:16 AM

native/chat/chat-thread-list.react.js
34 ↗	(On Diff #31151)	I think it should be fine because Flow types get "dropped" at runtime, but I can still move the type over to `ChatThreadListSearch` in a separate diff if that seems beneficial?

rebase after resolve merge conflict

Harbormaster completed remote builds in B22682: Diff 31210.Sep 18 2023, 10:38 AM

Closed by commit rCOMMe9e030d6493b: [native] Move rest of Search views to `ChatThreadListSearch` (authored by atul). · Explain WhySep 18 2023, 11:00 AM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMe9e030d6493b: [native] Move rest of Search views to `ChatThreadListSearch`.

tomek added inline comments.Sep 19 2023, 1:05 AM

native/chat/chat-thread-list.react.js
34 ↗	(On Diff #31151)	We should try to reduce the number of cycles we have and try to not introduce new ones. I think that if it's quick, we should fix it.