Page MenuHomePhabricator

[web] Creating typeahead overlay in ChatInputbar
ClosedPublic

Authored by przemek on Nov 24 2022, 8:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 8:37 PM
Unknown Object (File)
Sun, Nov 24, 8:33 PM
Unknown Object (File)
Sun, Nov 24, 7:47 PM
Unknown Object (File)
Sun, Nov 24, 7:27 PM
Unknown Object (File)
Sun, Nov 24, 5:48 PM
Unknown Object (File)
Fri, Nov 22, 8:00 AM
Unknown Object (File)
Fri, Nov 22, 8:00 AM
Unknown Object (File)
Fri, Nov 22, 8:00 AM

Details

Summary

Final diff for MVP typeahead for web.
We lack some nice UX from design, but it can be done in future diffs after
we implement typeahead on native.
Nice to haves (that we don't have yet):

  • keyboard support (choosing with arrows, confirming with Enter, cancelling with Esc), for now you gotta click username with your mouse
  • @ing people that are not in a channel thread would create a modal asking if you want to invaite them

See a video for how it feels and works:

Test Plan

See a video above.
I played with it a lot during development and it feels alright, but would appreciete any feedback after it is landed and you can test it yourself.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/chat/chat-input-bar.react.js
527 ↗(On Diff #18822)

typo

528 ↗(On Diff #18822)

I think we need a more specific name here

tomek requested changes to this revision.Nov 25 2022, 3:28 AM

Looks great!

web/chat/chat-input-bar.react.js
77 ↗(On Diff #18822)

It would be better to transform the match into an object with meaningful name and props

134–138 ↗(On Diff #18822)

What is the purpose of it?

146–152 ↗(On Diff #18822)

Why do we need this? Can we merge it with the previous if?

364–365 ↗(On Diff #18822)

What is the reason behind deleting this comment?

367–375 ↗(On Diff #18822)

Are there other actions that result in changing cursor position, e.g. using keyboard arrows?

523–526 ↗(On Diff #18822)

We can move it into the memo

This revision now requires changes to proceed.Nov 25 2022, 3:28 AM
przemek marked 7 inline comments as done.
przemek edited the summary of this revision. (Show Details)

Responded to inlines and fixed code.

web/chat/chat-input-bar.react.js
134–138 ↗(On Diff #18822)

We're setting caret position, when user changes threads. It enables us to load saved caret position when jumping between threads. We have to set both start and end of selection to the same number as we don't want any selected text at start.

146–152 ↗(On Diff #18822)

We need to change caret position in textarea after we choose user to mention. Draft is edited (we paste remaining part of chosen username that wasn't typed previously). this.textarea.selectionStart === this.textarea.selectionEnd part of condition is there to prevent situation where we want to select some text, but first click (start of selection) sets inputState.textCursorPosition, second click sets it again and then both start and end are set in the code above. This way text selecting can work independently of mentioning other users. We can't merge it with previous if as they're serving different purposes and require different conditions. We might create util function to call instead of the whole block:

this.textarea?.setSelectionRange(
        inputState.textCursorPosition,
        inputState.textCursorPosition,
        'none',
      );
367–375 ↗(On Diff #18822)

Currently not, as using keyboard was out of scope for this stack of diffs, but when we add it, we should probably refactor setting cursor to selectionStart as a separate function. We only do that on text change, clicks and choosing user (from list of users in overlay) to mention .

Realized too late that it also should be put inside useMemo.

tomek requested changes to this revision.Nov 25 2022, 9:17 AM
tomek added inline comments.
web/chat/chat-input-bar.react.js
80–82 ↗(On Diff #18844)

Do we need to repeat matched in every field when the type name also contains matched? Maybe we can simply delete that?

146–152 ↗(On Diff #18822)

I'm not sure if I'm missing something, but the code inside these two ifs look the same, so why we can't merge them? It's a simple code transformation:

if (a) {
  doX();
}
if (b) {
  doX();
}

into

if (a || b) {
  doX();
}

Also the fact that we're using this.props.inputState in one and inputState in another is confusing.

364–365 ↗(On Diff #18822)

Reminder about this comment.

367–375 ↗(On Diff #18822)

When talking about keyboard arrows I wasn't thinking about vertical ones. What happens when a user decides to use left or right arrow? Are we going to update the cursor position in the state?

This revision now requires changes to proceed.Nov 25 2022, 9:17 AM
przemek marked 5 inline comments as done.

Responded to suggestions.

web/chat/chat-input-bar.react.js
146–152 ↗(On Diff #18822)

You're right, obviously, I think in some previous versions, codes inside were different, thus they needed to be separeted.

364–365 ↗(On Diff #18822)

Sorry, I deleted that comment, as it was few years old, and that feature is already implemented. We only show send button, when draft is nonempty

367–375 ↗(On Diff #18822)

I had some troubles with making it work earlier while working on it. Added onSelect event handler for textarea and it works now. We update state on user using arrows.

tomek added inline comments.
web/chat/chat-input-bar.react.js
80–82 ↗(On Diff #18921)

Make these readonly

137–144 ↗(On Diff #18921)

Inside the if body, we can skip optional chaining ?. as textarea is non null.

Probably inside the condition we can do the same - we just have to check this.textarea && before any other statement.

139 ↗(On Diff #18921)

Why do we have to check the selection?

This revision is now accepted and ready to land.Nov 29 2022, 4:27 AM
przemek marked 3 inline comments as done.

Fixed readonlys

web/chat/chat-input-bar.react.js
139 ↗(On Diff #18921)