Page MenuHomePhabricator

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

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



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

atul published this revision for review.Thu, Sep 14, 1:25 PM
atul added inline comments.
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?

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

243 ↗(On Diff #31151)

Ah gotcha thanks for explaining, that makes a lot more sense.

tomek added inline comments.
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.Mon, Sep 18, 3:46 AM
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

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.