move onChangeSearch to lib/hooks/ from native and web.
Details
Details
Use the search sidebar feature in both native and web and confirm functionality still works as expected.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Looks close, left some thoughts/suggestions inline. Only thing I think needs to be addressed before accepting is changing onChangeSearchText return type from mixed -> void
| lib/hooks/search-sidebars.js | ||
|---|---|---|
| 23 ↗ | (On Diff #12971) | Shouldn't this be void instead of mixed? Let me know if I'm missing something (fwiw arc patch'd, made change, and there were no issues with flow) |
| native/chat/sidebar-list-modal.react.js | ||
| 38 ↗ | (On Diff #12971) | Thoughts on naming this something like onChangeSearchInputValue or onChangeSearchInputText or something? Just to make it clear that this callback is "triggered" when the value of the Input component changes, and not when the value of the searchState text changes? Don't feel strongly, up to you As an aside, since this callback is about handling an input event "within" the component... would it make more sense to leave the onChangeSearchText callback inside the component? Even though there's some repetition, it might keep related things closer? |
| 68–71 ↗ | (On Diff #12971) | Should we instead replace this with: onChangeSearchText('');? Then we can avoid using setSearchState "directly" and make onChangeSearchState the "API" for dealing with searchState? If we do this in web as well (in clearQuery), we could remove setSearchState from the object returned by useSearchSidebars? |
| web/modals/chat/sidebar-list-modal.react.js | ||
| 64 ↗ | (On Diff #12971) | Should we instead replace this with: onChangeSearchText('');? Then we can avoid using setSearchState "directly" and make onChangeSearchState the "API" for dealing with searchState? |
| lib/hooks/search-sidebars.js | ||
|---|---|---|
| 23 ↗ | (On Diff #12971) | Returning mixed is common for a callback type... it's a way of saying "I don't care what your callback returns, it can return whatever... I'm just going to ignore it" |
| web/modals/chat/sidebar-list-modal.react.js | ||
|---|---|---|
| 64 ↗ | (On Diff #12971) | Yeah, that makes sense. Could I do that in a following diff. In |
| web/modals/chat/sidebar-list-modal.react.js | ||
|---|---|---|
| 64 ↗ | (On Diff #12971) | Yeah sounds good, should prob make a linear task for it |
| native/chat/sidebar-list-modal.react.js | ||
|---|---|---|
| 38 ↗ | (On Diff #12971) | I just changed everything to onChangeSearchInputText I think that made the most sense. |
| 68–71 ↗ | (On Diff #12971) | This gets changed a bit over in: https://phabricator.ashoat.com/D4060. This functionality gets handled in the https://github.com/CommE2E/comm/blob/master/web/modals/search-modal.react.js#L9 component. |
| native/chat/sidebar-list-modal.react.js | ||
|---|---|---|
| 68–71 ↗ | (On Diff #12971) | talked to @atul IRL. This gets handled in https://phabricator.ashoat.com/D4060, and is okay. |