Details
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. Checked that if the query is cleared "Your search results will appear here!" message is displayed.
Checked that if the query is not cleared between the searches everything works - only the results of the second query are displayed.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/search/message-search.react.js | ||
|---|---|---|
| 67 | What's the point of this wrapping View? | |
Probably not worth doing now, but I think it would be more clear to the reveiwer if you had put D7774 before D7773. Then you could squash this into D7773
In D7774 I needed to have the MessageSearch route, to be able to render MessageResult. I could have split that diff, but I thought it didn't make much difference which one I split
Can you explain more about the distinction between MessageSearch and MessageSearchContent? Wondering why they're separated, and what each is responsible for
They are separated so that when there is no query, all of the code for fetching and parsing search results doesn't get called (since hooks cannot be called conditionally). Additionally MessageSearch takes care of cleaning up the results and/or the query when user does something like navigates back or changes the query. Search content is just meant to fetch and display things, not handle interactions.
since hooks cannot be called conditionally
To be honest, I think calling the search functionality from a hook instead of from a callback is the wrong approach. It leads to issues like ENG-3896, since data being recalculated can lead to the search being performed again. More details in my comment on D7774, but I suspect after applying my feedback there it might make sense to merge MessageSearch and MessageSearchContent.