Page MenuHomePhabricator

[web] Keyboard support for typeahead [11/13] - Use callbacks to interact with overlay
ClosedPublic

Authored by przemek on Dec 27 2022, 5:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 13, 8:54 PM
Unknown Object (File)
Thu, Mar 7, 7:17 AM
Unknown Object (File)
Thu, Mar 7, 7:17 AM
Unknown Object (File)
Thu, Mar 7, 7:16 AM
Unknown Object (File)
Thu, Mar 7, 1:36 AM
Unknown Object (File)
Thu, Mar 7, 1:36 AM
Unknown Object (File)
Thu, Mar 7, 1:36 AM
Unknown Object (File)
Wed, Mar 6, 1:06 AM

Details

Summary

Using callback functions to change chosen button, accept it or close overlay.

Test Plan

Tested how it all fits together. Results on a video (currently without scrolling overlay content - added in next diff):

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/chat/chat-input-bar.react.js
419–422 ↗(On Diff #20178)

I think the destructure syntax can be used here, but however you prefer. I find this to look cleaner

przemek marked an inline comment as done.

Destructure as Inka suggested.

web/chat/chat-input-bar.react.js
419–422 ↗(On Diff #20178)

Right, I haven't thought about using it in this context, thanks for suggestion!

tomek requested changes to this revision.Jan 4 2023, 6:18 AM
tomek added inline comments.
web/chat/chat-input-bar.react.js
425–451 ↗(On Diff #20513)

You can reduce duplication in this code by creating a map from key to function and using it instead of else if chain. Something like the included code, but it should be possible to simplify it even further.

web/utils/typeahead-utils.js
127 ↗(On Diff #20513)
127–129 ↗(On Diff #20513)

Why only one prop has overlay in name? Do we need it?

129 ↗(On Diff #20513)

Can we keep it next to setChosenPositionInOverlay?

133–141 ↗(On Diff #20513)

It should work and doesn't need to be changed, but it would be less fragile if the logic was using keys instead of idx.

This revision now requires changes to proceed.Jan 4 2023, 6:18 AM
przemek marked 5 inline comments as done.

Responded to inlines and fixed code.
Tested everything once again.

web/chat/chat-input-bar.react.js
425–451 ↗(On Diff #20513)

Thanks for the suggestion! I felt like it is really repetitive, but couldn't find a way to simplify it.

web/utils/typeahead-utils.js
127–129 ↗(On Diff #20513)

I forgot to rename chosenActionPosition argument. Renamed it so it match name from callsite and correspond to its setter.

133–141 ↗(On Diff #20513)

I'm not sure if we can use keys here. They are based on user id: key: suggestedUser.id, as you can see in getTypeaheadTooltipActions function above. And chosenPositionInOverlay is simply index in list which doesn't really correspond to user id.

This revision is now accepted and ready to land.Jan 5 2023, 7:37 AM