Details

Reviewers

tomek
inka
ginsu
kamil
michal
ashoat

Commits

rCOMM0e1154b52e4a: [8/n] Native Typeahead - Create tooltip component in chat input bar

Summary

Creating typeahead in ChatInputBar. There is one iOS bug/bad UX described and showed in a thread here: https://linear.app/comm/issue/ENG-2367/typeahead-in-tooltip-for-mentions-for-native

I'm putting up diffs regardless so they can be reviewed, but I can't find a way to fix it.
Adding @ashoat as blocking as the diff contains currently unsolved bug.

Test Plan

Checked if app builds.
Video android:

Video iOS (showing bug with cursor not moving past name)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• przemek created this revision.Jan 25 2023, 7:29 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptJan 25 2023, 7:29 AM

• przemek added a parent revision: D6369: [7/n] Native Typeahead - Introduce tooltip component.Jan 25 2023, 7:29 AM

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

• przemek attached a referenced file: F344238: Screen Recording 2023-01-25 at 16.11.28.mov. (Show Details)

• przemek attached a referenced file: F344237: android.mov. (Show Details)

• przemek added a reviewer: ashoat.

• przemek edited the summary of this revision. (Show Details)Jan 25 2023, 7:32 AM

Harbormaster completed remote builds in B15638: Diff 21319.Jan 25 2023, 7:43 AM

• przemek requested review of this revision.Jan 25 2023, 7:43 AM

ashoat resigned from this revision.Jan 25 2023, 12:38 PM

ashoat added inline comments.

native/chat/chat-input-bar.react.js
478–517 ↗	(On Diff #21319)	Not familiar with this logic, defer to others Personally find the ternary messy, feels like it would be cleaner to have an `if (typeaheadRegexMatches) {` branch, or maybe move all of this to a function so you could `return null` whenever you need to, and otherwise just proceed with normal function calls

Small fix

Harbormaster completed remote builds in B15786: Diff 21532.Jan 29 2023, 2:49 PM

It might be a good idea to check how this code performs on a slower Android device when there are a lot of matches and a text is long. It seems like memoization could be a good idea here - in class components we can achieve that by using createSelector, chat-thread-list contains an example.
If the performance is ok, we don't have to complicate the code.

native/chat/chat-input-bar.react.js
478–481 ↗	(On Diff #21532)	How about the case where selection start is different than its end?
478–488 ↗	(On Diff #21532)	We can optimize this code. There's no point in slicing the string when we're not going to check if it matches the regex.

This revision now requires changes to proceed.Jan 30 2023, 3:52 AM

I've tested the app in simulator, which is not ideal, but probably less time consuming than dealign with physcial build right now, and the performance seems identical using master branch, or branch with typeahead feature. I've pasted huge message into chat input bar, and hasn't spotted any lag when it comes to typeahead showing or updating.

native/chat/chat-input-bar.react.js
478–481 ↗	(On Diff #21532)	It might be a bit inexplicit, but this test: /\s/.test(this.state.text[this.state.selection.start]) failed, when the first selected character was non-whitespace. I'm not sure what effect we want to achieve with selection near @ symbol. For now I'll add condition so we only display typeahead when start === end and create linear task for that

Harbormaster completed remote builds in B15869: Diff 21661.Jan 31 2023, 3:06 AM

• przemek planned changes to this revision.Jan 31 2023, 3:30 AM

small fix - the condition was wrong

Harbormaster completed remote builds in B15873: Diff 21665.Jan 31 2023, 4:47 AM

tomek requested changes to this revision.Jan 31 2023, 5:23 AM

tomek added inline comments.

native/chat/chat-input-bar.react.js
479–486	This ternary is complicated - could you replace this with an `if`? Probably most of the ternaries from here should be replaced, according to https://www.notion.so/commapp/Use-ternary-conditionals-sparingly-f4ba44a10259403592a1d15440a9847e

This revision now requires changes to proceed.Jan 31 2023, 5:23 AM

ashoat added inline comments.Jan 31 2023, 6:30 AM

native/chat/chat-input-bar.react.js
479–486	As I pointed out earlier, I think this can mostly be replaced with a `if (typeaheadRegexMatches) {`

Mostly refactor. Respond to reviewers.

Harbormaster failed remote builds in B15892: Diff 21693!Jan 31 2023, 10:06 AM

Harbormaster completed remote builds in B15892: Diff 21693.Jan 31 2023, 11:13 AM

ashoat requested changes to this revision.Jan 31 2023, 12:06 PM

ashoat added inline comments.

native/chat/chat-input-bar.react.js
557–582 ↗	(On Diff #21693)	Combine these, reduce conditionals

This revision now requires changes to proceed.Jan 31 2023, 12:06 PM

Combined conditionals. That logic should be refactoed into seperate functions as it's quire similar to the web counterpart anyway. I've created the task for that: https://linear.app/comm/issue/ENG-2891/refactor-some-of-the-native-typeahead-creation-logic-and-make-web
I'd rather do it after native typeahead is landed as it has higher prio.

Harbormaster completed remote builds in B15919: Diff 21727.Feb 1 2023, 1:57 AM

Readability is getting a lot better. Please address inline comment before landing

native/chat/chat-input-bar.react.js
534–555 ↗	(On Diff #21727)	These should be combined into `if` + `else if`

if -> if else.
Rebase.

Harbormaster completed remote builds in B15923: Diff 21734.Feb 1 2023, 3:59 AM

• przemek added inline comments.Feb 1 2023, 7:12 AM

native/chat/chat-input-bar.react.js
545–553 ↗	(On Diff #21734)	only place where `androidPreviousText` is used

You changed behavior in the last update. Previously if both conditions were valid, we would "default" to the this.androidPreviousText condition. Now we're "defaulting" to the condition. Is this change intentional, and does it potentially introduce any issues?

This revision now requires changes to proceed.Feb 1 2023, 10:33 AM

My above comment needs to be addressed before landing (maybe by switching order of conditions), but switching to an accept so it stays on @tomek's queue

This revision now requires review to proceed.Feb 1 2023, 10:34 AM

Now we're "defaulting" to the condition. Is this change intentional, and does it potentially introduce any issues?

After simplifying the condition it seems like the inverted logic shouldn't work properly.

@przemek please remember to test this thoroughly after the conditions are changed.

native/chat/chat-input-bar.react.js
534–553 ↗	(On Diff #21734)	It seems like this can be significantly simplified by extracting the common part. This will also default to the Android text.

This revision now requires changes to proceed.Feb 2 2023, 5:21 AM

• przemek planned changes to this revision.Feb 2 2023, 7:23 AM

Moved the logic to separate function. It's places in lib, as it should also be used in web. We have task for that: https://linear.app/comm/issue/ENG-2891/refactor-some-of-the-native-typeahead-creation-logic-and-make-web
I've tested a lot and it works we default to using state, if values in class fields are null.

• przemek planned changes to this revision.Feb 2 2023, 7:34 AM

Harbormaster completed remote builds in B16007: Diff 21851.Feb 2 2023, 7:38 AM