Page MenuHomePhabricator

[web] Additional state required for keyboard support for typeahead.
ClosedPublic

Authored by przemek on Dec 6 2022, 2:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Wed, Apr 3, 5:53 PM
Unknown Object (File)
Mar 5 2024, 1:26 AM
Unknown Object (File)
Feb 18 2024, 8:52 AM

Details

Summary

We need extra state for controlling currently chosen button in typeahead overlay and for user actions (Escape for closing it, or Enter for accepting).

Test Plan

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

tomek requested changes to this revision.Dec 6 2022, 3:33 AM
tomek added inline comments.
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?

This revision now requires changes to proceed.Dec 6 2022, 3:33 AM

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.

tomek requested changes to this revision.Dec 12 2022, 2:18 AM
tomek added inline comments.
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.

This revision now requires changes to proceed.Dec 12 2022, 2:18 AM
przemek marked 5 inline comments as done.

Responded to inlines and fixed code.

przemek added inline comments.
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.

This revision is now accepted and ready to land.Dec 14 2022, 7:27 AM