Issue: https://linear.app/comm/issue/ENG-3163/create-the-search-screen-for-message-searching
Add a component that will be displayed in the search screen when there is a query typed into the search bar. The logic is pretty similar to pinned messages : D7673
Some navigation types had to be updated to allow using MessageResult in this component.
Details
Details
- Reviewers
• kuba kamil bartek ashoat - Commits
- rCOMM4b9285c9f6a5: [native] Add MessageSearchContent
Tested with the next diff - displayed it the search screen and checked that the messages are fetched and displayed correctly. Checked that the "End of results" and "No results..." messages display correctly. Checked the case when
the first fetch returns all results as well as the case when the user needs to scroll to fetch more messages a couple of times.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/search/search-content.react.js | ||
|---|---|---|
| 189 ↗ | (On Diff #26329) | Is it possible to factor out the shared logic between this function and getOldestNonLocalMessageID? |
Comment Actions
Address https://linear.app/comm/issue/ENG-3882/reaction-not-showing-in-search-screen - now all search results are being passed to messageListData, so that if a user reacts to a message in the search screen, the
reaction gets shown
| native/search/search-content.react.js | ||
|---|---|---|
| 189 ↗ | (On Diff #26329) | Is it worth it? It's a simple function, and making the logic shared would require making this code more complex since the types used by these functions are different and they check some property that the other type doesn't have (itemType), or doesn't make sense to check in the other case (id.startsWith(localIDPrefix)). The only thing they share is looping backwards |
| native/search/search-content.react.js | ||
|---|---|---|
| 189 ↗ | (On Diff #26329) | Okay, fair point |
| native/search/search-content.react.js | ||
|---|---|---|
| 60 ↗ | (On Diff #26419) | Doing this in an effect feels weird to me, as opposed to doing it in the callback where we fetch the results from the server. I have two concerns:
Is there a way we can make all of this happen in a callback? For instance, what if useSearchMessages was passed a callback, and useSearchMessages would trigger that callback when it received new search results? |
| 80 ↗ | (On Diff #26419) | Nit |
| 101 ↗ | (On Diff #26419) | Why do we only add the loader if it's present in chatMessageInfos? I don't think there's any relation between the loader in chatMessageInfos (which is present if startReached === false in Redux) and the loader being used here (which is present if the search results have not been exhausted) |
| native/search/message-search.react.js | ||
|---|---|---|
| 180 ↗ | (On Diff #26732) | Why is this newline character here? This is really bad for responsiveness... you seem to be assuming the width of the device. I think you want to apply padding instead? |
Comment Actions
Passing back to your queue, in part for feedback above, but mostly for the changes I requested in D7771
| native/search/message-search.react.js | ||
|---|---|---|
| 42–44 | Can you get rid of the wrapper function and just pass clearQuery in directly? The wrapper function doesn't appear to be doing anything | |