Preparing for native typeahead.
Adding selection to state so we can control it in input bars. There was a problem on iOS desribed here: https://linear.app/comm/issue/ENG-2367#comment-8e0f3042.
It was solved by adding controlSelection flag. We set it to false initially so we don't control
selection. It will be set true when we want to manipulate selection when inserting usernames after tapping into typeahead buttons (incoming diffs)
Details
Details
Checked if app builds.
Checked if text input works corretly (both Android and iOS)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 629–633 ↗ | (On Diff #21317) | This was a hack I came up with that improves behavior on iOS. iOS doesn't have controlled selection very well, so basically we only control the selection when we need to (when we are changing the selection). We don't need to control the selection in this case, so we don't |
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 629–633 ↗ | (On Diff #21317) | Quite the opposite. We only want to set state here, but we don't want to control selection after native update of selection as it causes flickering. We only set controlSelection to true if we want to manipulate selection. |
Comment Actions
Can we avoid putting these hacks in ChatInputBar? When we have hacks like this, it's best to factor them out into their own component, eg. ClearableTextInput, ChatList, etc. Leaving them in ChatInputBar violates separation of concerns, and makes the file really messy.
Could we create something like SelectableTextInput that wraps ClearableTextInput and fixes the issues with selection? We could also fork it into a .ios.js and .android.js file, which would allow us to keep the hacks separate
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 178–205 ↗ | (On Diff #21690) | I don't think this text explains what's going on very well what's going on, and I also think it's lengthier than it needs to be. I don't have time to rewrite it for you unfortunately. Did you make sure you rewrote this eg. 3x times? You should never go with the first iteration of something like this... it's necessary to rewrite it multiple times to get a good result. |
| native/components/clearable-text-input.js | ||
| 9–13 ↗ | (On Diff #21690) | Can these be made $ReadOnly? |
| 12–13 ↗ | (On Diff #21690) | I don't understand why you specified these two types here.
The other three props here all have good reasons for being here. textInputRef doesn't appear in TextInputProps, and the other two (onChangeText and value) are optional in TextInputProps, but required for us I don't see any such "good reasons" for you including these here. Did you make sure it was necessary? |
Comment Actions
I've tried out a lot of solutions. Process and thoughts documented here: https://linear.app/comm/issue/ENG-2890/issue-with-typeahead-flickering-while-the-user-types
I decided to go with a solution I initially came up for Android. It's described in a comment in this commit.
| native/components/clearable-text-input.js | ||
|---|---|---|
| 12–13 ↗ | (On Diff #21690) | Thanks for explanation. Now I understand reasoning about the other ones. I'm not sure now, but initially I had problems with flow without specifying them. Maybe I changed something without revising lines above later on. I tested everything without them and it works. |
Comment Actions
With the state of this work I think it's critical for me to actually patch it and play around with it in my local environment, but I didn't get time to do that today. @tomek, would be great if you could take a first pass
| native/components/clearable-text-input.react.ios.js | ||
|---|---|---|
| 157 ↗ | (On Diff #21852) | What is the point of this change? |
| 171 ↗ | (On Diff #21852) | What is the point of this change? |
| native/components/clearable-text-input.react.js | ||
| 63 ↗ | (On Diff #21852) | What is the point of this change? |