Page MenuHomePhabricator

[native] Introduce naive `ChatThreadListSearch` component
ClosedPublic

Authored by atul on Sep 14 2023, 12:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 30, 8:44 PM
Unknown Object (File)
Sun, Jun 30, 7:18 AM
Unknown Object (File)
Sun, Jun 30, 5:37 AM
Unknown Object (File)
Sat, Jun 29, 3:34 PM
Unknown Object (File)
Thu, Jun 27, 6:40 PM
Unknown Object (File)
Thu, Jun 27, 6:40 PM
Unknown Object (File)
Thu, Jun 27, 6:40 PM
Unknown Object (File)
Thu, Jun 27, 6:32 PM
Subscribers

Details

Summary

This diff starts moving things from ChatThreadList to ChatThreadListSearch in a pretty naive/cut-and-paste way. Breaking into steps so I can make sure that things continue to work as expected after each change. Originally made all the changes at once and ran into weird issues I couldn't troubleshoot, so hopefully doing this incrementally will make it easier to narrow down.

This diff involves moving JUST the Search component to ChatThreadListSearch and ensuring that onChangeText, onBlur, and additionalProps get passed in and work as expected.

Test Plan

Search experience continues to work as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested review of this revision.Sep 14 2023, 12:57 PM

accepting, but please take a look at the inline comments

native/chat/chat-thread-list-search.react.js
10–11 ↗(On Diff #31150)

Not sure if this will get updated in a subsequent diff, but just a reminder to use mixed instead of void

32 ↗(On Diff #31150)

assuming that all these styles being copied to here will eventually get removed in chat-thread-list in a subsequent diff

This revision is now accepted and ready to land.Sep 14 2023, 1:10 PM
native/chat/chat-thread-list-search.react.js
10–11 ↗(On Diff #31150)

Is there a reason to prefer mixed instead of void? Isn't void more precise since it's explicitly stating that the function doesn't return anything vs. mixed which would allow the function to return number, string, undefined, etc?

32 ↗(On Diff #31150)

Yeah this is handled later in the stack

native/chat/chat-thread-list-search.react.js
10–11 ↗(On Diff #31150)

This was some feedback I got from @ashoat in D7643

This pattern of passing functions directly in instead of creating aliases is always easier if you type functions that take other functions as input (like useUploadSelectedMedia here) in a more permissive way, but letting them take any function that returns anything (mixed) instead of insisting that those functions return void specifically. In this case it didn’t matter because setState functions return void (see here) but it’s still a good pattern to follow.

native/chat/chat-thread-list-search.react.js
10–11 ↗(On Diff #31150)

if you type functions that take other functions as input (like useUploadSelectedMedia here) in a more permissive way

That makes sense, but this isn't a function that takes another function as input. Does that feedback still apply?

(CC @ashoat)

native/chat/chat-thread-list-search.react.js
10–11 ↗(On Diff #31150)

Yeah I think it still counts… a component taking props is similar to a function taking input

native/chat/chat-thread-list-search.react.js
10–11 ↗(On Diff #31150)

Imagine you want to pass an async function to onBlur. This forces you to wrap it in another function

If we don’t care about the result on onBlur, we should allow mixed

native/chat/chat-thread-list-search.react.js
10–11 ↗(On Diff #31150)

Gotcha, I'll update this diff w/ the changes.

type fn return values as mixed

This revision was landed with ongoing or failed builds.Sep 17 2023, 4:15 PM
This revision was automatically updated to reflect the committed changes.