Paths

Table of Contentst

[web] Introducing keyboard support for typeahead
AbandonedPublic
Actions

Authored by • przemek on Dec 9 2022, 10:23 AM.

Details

Reviewers

kamil
ginsu
inka
tomek

Summary

Keyboard support for typeahead is here.
The diff is rather big, but it would be trouble some to split it up as changes to utility functions
would affect already working code. So it was either to create non-building diffs or create one big diff.

In utils it's mainly adding new functions and some refactor of others.

In chat-input-bar we use state and callbacks added in previous diff to control overlay.

Most interesting changes in typeahead-tooltip which I'll try to explain in inlines.

According to last retro I'm not adding final blocking reviewer until I get initial review from one of those.

Test Plan

Played with it.

User can move using arrows, cancel using Escape and accept using Enter or Tab

Video:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• przemek created this revision.Dec 9 2022, 10:23 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 9 2022, 10:23 AM

• przemek added a parent revision: D5819: [web] Add other <button> props to Button..Dec 9 2022, 10:25 AM

Harbormaster completed remote builds in B14079: Diff 19271.Dec 9 2022, 10:35 AM

• przemek requested review of this revision.Dec 9 2022, 10:35 AM

• przemek edited the test plan for this revision. (Show Details)Dec 13 2022, 2:04 AM

• przemek attached a referenced file: F282546: Screen Recording 2022-12-09 at 19.18.07.mov. (Show Details)

• przemek edited the test plan for this revision. (Show Details)

ashoat added inline comments.Dec 13 2022, 9:25 AM

web/chat/chat-input-bar.react.js
445 ↗	(On Diff #19271)	We should handle the easy case first and early-exit. See here
445–446 ↗	(On Diff #19271)	You should never have an `if` nested in an `else`. This is an `else if`. Please try to reduce indentation in your code!
web/chat/typeahead-utils.js
93–100 ↗	(On Diff #19271)	A lot of parameters here... seems like it would be clearer at the callsite if you turn this into a object so that the params are named

Addressed Ashoat's comments.

Harbormaster completed remote builds in B14161: Diff 19365.Dec 15 2022, 1:46 AM

• przemek added a child revision: D5876: [web] Refactor and renaming of typeahead utils..Dec 15 2022, 4:11 AM

ashoat added inline comments.Dec 15 2022, 7:11 AM

web/chat/chat-input-bar.react.js
425 ↗	(On Diff #19365)	Please expand this using "De Morgan's Law"

Addressed comment.

Harbormaster completed remote builds in B14181: Diff 19399.Dec 15 2022, 9:20 AM

Starting reviewing from middle of the stack so adding someone with more context as blocking but I left some comments inline

web/chat/chat-input-bar.react.js
425 ↗	(On Diff #19399)	I am wondering if this can be assigned to a variable with a meaningful name, and then maybe it can be merged with nested `if` to remove indentation?
web/chat/typeahead-tooltip.react.js
49 ↗	(On Diff #19399)	`animation` makes me feel like it's state for animation itself, since it's `boolean` I will suggest `isAnimating` or something similar, its's up to you
56 ↗	(On Diff #19399)	this dependency is not needed, `setAnimation` will not change
58 ↗	(On Diff #19399)	can be shortened by removing `return` and curly brackets
168 ↗	(On Diff #19399)	Do we need to check `!actions`? Looks like it should be a type of `$ReadOnlyArray<TypeaheadTooltipAction>` so I think checking for length is enough
web/chat/typeahead-utils.js
92 ↗	(On Diff #19399)	Let's start type names with capitalize letter
144 ↗	(On Diff #19399)	I think it'll be more readable to use one prop and then destruct `{ key, onClick, actionButtonContent }` inside the function but I've seen code like this in codebase so it's up to you
180–190 ↗	(On Diff #19399)	And with that change, we can get rid of `newScrollTop` variable
230–235 ↗	(On Diff #19399)	Maybe this operation could be divided into smaller chunks with variables and meaningful names to be more readable? but it's up to you

One more question about the design, I checked Figma and looks like the outline shouldn't break scroll line, is it intended?

Screenshot 2022-12-16 at 10.50.24.png (255×359 px, 20 KB)

Addressed issues in code and answered to comments.

The thing about scrollbar is intended, we discussed it in linear task with dave and agreed on it being better. Moving all tiles to the left created weird empty space on the right when scrollbar was not visible.

web/chat/chat-input-bar.react.js
425 ↗	(On Diff #19399)	I tried that, but after changing code to this: const isOverlayHidden = !this.props.inputState.typeaheadState.isVisible \|\| !accept \|\| !close; if (isOverlayHidden) { if (event.key === 'Enter' && !event.shiftKey) { event.preventDefault(); this.send(); } } else if (event.key === 'Enter' \|\| event.key === 'Tab') { event.preventDefault(); accept(); close(); } Flow says that `accept` and `close` can be null, so it's not checking them, so I went with solution you can see in the code
web/chat/typeahead-tooltip.react.js
49 ↗	(On Diff #19399)	Right, changed
168 ↗	(On Diff #19399)	You're right, changed. Also thanks to that nit I found a bug, which resulted in changes in lines 140-141. When we couldn't match any user with some prefix but pressed Enter message wouldn't get sent, but it would rather try to @ non existing user. Should be fixed now.
web/chat/typeahead-utils.js
144 ↗	(On Diff #19399)	yeah, I agree, destructing inside of function looks better
230–235 ↗	(On Diff #19399)	I haven't created extra variables, but simplified expression. Half of it was actually unnecessary. Basically we just need: newValue = oldValue - floor(oldValue / base) * base which returns positive modulo both por positive and negative numbers

• przemek marked an inline comment as done.Dec 16 2022, 2:53 AM

Harbormaster completed remote builds in B14198: Diff 19427.Dec 16 2022, 3:03 AM

Some questions about the test plan:

What happens when a user hovers over an option and then clicks an arrow?
What happens when a user is selected and someone is added to a chat?
What happens when a user is selected and someone removes him from a chat?

Handling all these corner cases might be time consuming, but we should at least check if the experience isn't completely broken.

Overall, this diff is huge and should've been split into multiple ones. Some options included: introducing new functions in separate diffs, modifications to state, modifications to rendering based on state and finally, updating the state based on actions.

web/chat/chat-input-bar.react.js
114–122	`isVisible` suggests that typeahead will be visible, but in fact it only tells if it can be visible. It will be visible only if the regex also matches. So maybe rename it to `canBeVisible`? Also, what happens when a user starts typing `@...`, then hits enter and the continues writing? We should probably keep the typeahead hidden - is that what's happening?
430–449	Shouldn't we also check `this.props.typeaheadMatchedStrings`?
430–449	Is it guaranteed that the typeahead is visible in these branches?
443–444	Is this correct? Subtracting `1` from `0` gives `-1`
web/chat/typeahead-tooltip.react.js
49–56	`isAnimation` is a confusing name. We're setting it to true and it stays in this state until the unmount, which should mean that we're constantly animating something. It is probably not the case.
115–121
124–125	Do you think that having this additional variable is beneficial?
133–135	You can simplify this: a lambda that takes no argument and calls a function without argument can be simply replaced by that function. And the, the whole callback won't be needed.
145–163	Is this effect duplicated?
web/chat/typeahead-utils.js
147–156	It might be ok, but don't feel safe. Can we match by `key` instead? The possible issue is when a list of user changes (someone is added / removed from a chat). Are we sure that won't cause any issues?
229–233	What's happening here?

This revision now requires changes to proceed.Dec 16 2022, 4:55 AM

• przemek removed a child revision: D5876: [web] Refactor and renaming of typeahead utils..Dec 22 2022, 4:55 AM

Responding to questions but need to abandon revision to split diffs into smaller ones. Also I introduce huge changes to code so it would be beneficial to discuss in fresh diff

Responses about testing:
1 .Mouse disappears and user can move through options using arrows. If they move the cursor again it will show where it previously disappeared and change selection which is expected I guess.

They appear on a list and position is recalculated.
They disappear from the list and position is recalculated.

Position recalculation can be chaotic, so maybe it's good idea to always set it to the beginning on overlay when suggested users change?
It feels weird when you have typeahead presented and then it suddenly changes.
Maybe it is better to simply close it on users change?

You're right about the size of diff. It felt too bug, but on the other hand all things felt connected. I will do better next time.

web/chat/chat-input-bar.react.js
114–122	Agree, couldn't really find a better name and logic surrounding it was rather complicated. Probably you meant 'hits Escape' and not Enter. Well in this case after Escape typeahead would disappear, but after they type another letter and it would match some users it would show again. It may be undesirable behaviour but it should be put in separate task as it would be rather complicated and would require complex state to handle such situation.
430–449	The first condition: `if (!this.props.inputState.typeaheadState.isVisible \|\| !accept \|\| !close)` takes care of cases where typeahead doesn't exist. That's what Ashoat suggested in one of his first comments in this revision.
430–449	We don't need to check as without it set overlay wouldn't be created and thus accept and close wouldn't exist. As you've mention before this.props.inputState.typeaheadState.isVisible should be renamed to canBeVisible. That approach feels kinda bad as I'm treating accept and close existence (not being null) as a flag for typeahead actual presence Maybe I should just add another flag to TypeaheadState to represent current state of overlay instead of using accept & close callbacks.
443–444	That's the thing I described down below regarding `getTypeaheadChosenActionPosition`. We only calculate sum of ArrowUps and Downs and then calculate modulo, so we don't have to keep track on number of suggested users here, outside of the component.
web/chat/typeahead-tooltip.react.js
49–56	Changed it to isVisible,. I wanted to avoid same name for this local state needed for animation only and context state needed for logic outside of component
124–125	it is necessary, as without it, eslint requires entire inputState as dependency and that would result in endless rerendering loops. That's why us used that `setter` variable everywhere.
145–163	Yeah, sorry for that, probably artifact from merging changes and accepting both.
web/chat/typeahead-utils.js
147–156	I have checked edge cases you pointed out in main comment. Actions would be recalculated on users change, so they should always match to existing users.
229–233	We're calculating positive solution for chosenButtonNumber % actionsLength. `chosenButtonNumber` is not restricted by actions.length. So if we have 4 possible users to @ base on what we typed, if we keep pressing ArrowDown key chosenButtonNumber will increase indefinitely. I solved it in this way, because then I don't need access to number of actions outside of TypeaheadTooltip component. So we basically remember absolute sum of ArrowUps and Downs (and reset it to 0 every time component reappears) and only calculate modulo here