issues: https://linear.app/comm/issue/ENG-3466/add-some-state-for-keeping-the-search-query, https://linear.app/comm/issue/ENG-4109/add-search-button-to-search-modal
The query is saved only once the user has pressed "Search".
Paths
| Differential D8274 Authored by inka on Jun 21 2023, 5:38 AM.
Details Summary issues: https://linear.app/comm/issue/ENG-3466/add-some-state-for-keeping-the-search-query, https://linear.app/comm/issue/ENG-4109/add-search-button-to-search-modal The query is saved only once the user has pressed "Search". Test Plan (for the purpose of recording this I bound the pinned messages bar to message search modal)
Diff Detail
Event TimelineHerald added subscribers: tomek, ashoat. · View Herald TranscriptJun 21 2023, 5:38 AM2023-06-21 05:38:18 (UTC-7) inka added a parent revision: D8273: [web] Introduce onClearText to Search.Jun 21 2023, 5:38 AM2023-06-21 05:38:55 (UTC-7) inka edited the summary of this revision. (Show Details)Jun 21 2023, 5:42 AM2023-06-21 05:42:16 (UTC-7) inka attached a referenced file: F599595: Screen Recording 2023-06-21 at 14.40.15.mov. (Show Details) Harbormaster completed remote builds in B20378: Diff 27927.Jun 21 2023, 5:55 AM2023-06-21 05:55:36 (UTC-7) Comment Actions Not sure what the status of the "Search" button is, is the plan for it to be implemented in other search modals too, or is it just a one-off? Do you think it would make sense to add it (as an option) to the SearchModal?
inka added a child revision: D8275: [web] Make pressing enter initiate search.Jun 22 2023, 7:13 AM2023-06-22 07:13:38 (UTC-7) Comment Actions Also, the query won't be cleared if you manually remove text in the textbox (compared to clicking the X button) which I think I would find confusing. Comment Actions
Initially I implemented it like this, but because we want to remember the query in the context, using SearchModal made the code so complex, it was unreasonable. This modal is different from other search modals in that it fetches information from the server, not from redux, and it stores some information in the context. So I think it is reasonable that it requires a different implementation.
I will bring this up with Ted, but it does make sense to me personally Harbormaster failed remote builds in B20456: Diff 28038!Jun 23 2023, 3:18 AM2023-06-23 03:18:23 (UTC-7) Comment Actions Makes sense, thanks for explaining
This revision is now accepted and ready to land.Jun 23 2023, 3:37 AM2023-06-23 03:37:20 (UTC-7) Harbormaster failed remote builds in B20461: Diff 28043!Jun 23 2023, 5:50 AM2023-06-23 05:50:16 (UTC-7) Harbormaster failed remote builds in B20466: Diff 28048!Jun 23 2023, 8:25 AM2023-06-23 08:25:12 (UTC-7) Harbormaster completed remote builds in B20466: Diff 28048.Jun 26 2023, 7:38 AM2023-06-26 07:38:35 (UTC-7) Harbormaster completed remote builds in B20532: Diff 28149.Jun 27 2023, 3:56 AM2023-06-27 03:56:36 (UTC-7) Closed by commit rCOMM7f4b6ce88867: [web] Use message search context in message search modal (authored by • InkaSokolowska). · Explain WhyJun 27 2023, 4:25 AM2023-06-27 04:25:59 (UTC-7) This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 28156 web/app.react.js
web/modals/search/message-search-modal.css
web/modals/search/message-search-modal.react.js
|