We need extra state for controlling currently chosen button in typeahead overlay and for user actions (Escape for closing it, or Enter for accepting).
Details
App runs.
Extra states used in future diffs, where they will be tested.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
web/input/input-state-container.react.js | ||
---|---|---|
124–125 ↗ | (On Diff #19163) | What do you think about merging these into one object? They're closely related. |
1112–1116 ↗ | (On Diff #19163) | Why do you use a function when the argument isn't used? It would be simpler to pass an object directly to set state. |
web/input/input-state.js | ||
37 ↗ | (On Diff #19163) | What does type accepted mean? |
Idea changed a bit. Now all state regarding typeahead overlay is gatherd in one object.
It will keep callback accept and close so they can be called outside of overlay to perform actions on it.
web/input/input-state-container.react.js | ||
---|---|---|
80–81 ↗ | (On Diff #19270) | You can merge these |
531–541 ↗ | (On Diff #19270) | inputBaseStateSelector is memoized based on threadID because every selector in it depends on the id. Here we don't need to do that so using memoization doesn't make too much sense. |
543–551 ↗ | (On Diff #19270) | This memoization and selector doesn't make sense. It returns just the provided value, but in a really complicated way. Instead it should merge values provided (something like this:) |
553 ↗ | (On Diff #19270) | What's this? |
1127–1134 ↗ | (On Diff #19270) | We can use simplified syntax |
1295–1298 ↗ | (On Diff #19270) | To achieve the memoization of the result we should not spread the states. Instead we should pass them directly and then merge inside the selector. |
web/input/input-state-container.react.js | ||
---|---|---|
553 ↗ | (On Diff #19270) | I'm not sure what pushed my mind into territories of such abstraction. |
web/input/input-state-container.react.js | ||
---|---|---|
1295 ↗ | (On Diff #19309) | You can just initialize it to this and get rid of the else |
I think it is a really good idea to describe somewhere why we're using additional selectors. Probably a code comment is a place for it.