Page MenuHomePhabricator

[native, lib, web] [refactor] move text out of state
Changes PlannedPublic

Authored by benschac on May 16 2022, 2:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 12:22 PM
Unknown Object (File)
Sun, Mar 24, 5:13 PM
Unknown Object (File)
Sun, Mar 10, 9:57 PM
Unknown Object (File)
Fri, Mar 8, 6:34 AM
Unknown Object (File)
Feb 21 2024, 11:45 PM
Unknown Object (File)
Feb 21 2024, 9:54 PM
Unknown Object (File)
Feb 21 2024, 8:09 PM
Unknown Object (File)
Feb 21 2024, 7:01 PM

Details

Summary

to make search sidebars work with the SearchModal component it's state needs to be refactored to just the result set. This diff removes the text key/value and moves it into a React.useState which is later refactored in the following diff to get sidebar search data to work with SearchModal component.

Test Plan

Flow and eslint don't error. The sidebar search isn't functional in this diff but will be in the next diff.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/hooks/search-sidebars.js
91 ↗(On Diff #12775)

After implementing <SearchModal /> in the following diff, we no longer need clearQuery and it's removed in the next diff.

tomek requested changes to this revision.May 19 2022, 1:11 AM
tomek added inline comments.
lib/hooks/search-sidebars.js
15 ↗(On Diff #12826)

Why do we repeat text?

58 ↗(On Diff #12826)

We don't usually start variable names with onSomething - this name is used for callbacks.

native/chat/sidebar-list-modal.react.js
59 ↗(On Diff #12826)

I have some doubts about it. Currently, all the clients of the hook can set both text and search result. It would be a lot easier to reason about the state if only the text could be set and then the result would be updated by the hook. Do you think this can be achieved?

This revision now requires changes to proceed.May 19 2022, 1:11 AM