Added current cursor position in textarea input of chatInputBar to StateInput. It will be used in typeahead as we will need to track cursor position. Putting it in context provides us with extra benefit of saving cursor position when jumping between different chats and channels.
Code from this diff doesn't call anything them yet.
Details
Details
Runned web. Everything works fine.
New property and function are visible in chat-input-bar.reat.js via this.props.stateInput.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/input/input-state-container.react.js | ||
|---|---|---|
| 158–176 ↗ | (On Diff #18317) | Are you sure this code works correctly? Why do we assign pendingUploads to textCursorPositions and later textCursorPositions to stateUpdate.pendingUploads? Looks like a bug. |
| 488 ↗ | (On Diff #18317) | ?? syntax could be used |
| 1085–1090 ↗ | (On Diff #18317) | Shouldn't we clear the old state when a thread becomes realized (newThreadID !== threadID) |
Comment Actions
Responded to inlines.
| web/input/input-state-container.react.js | ||
|---|---|---|
| 488 ↗ | (On Diff #18317) | Right, also changed the line above. |
| 1085–1090 ↗ | (On Diff #18317) | It's already done in reassignToRealizedThreads function on props update. |
Comment Actions
We're storing cursor position for each thread. How does this work when switching between the threads? Do we really need to have it per thread, or is it enough to have a single value?
| web/input/input-state-container.react.js | ||
|---|---|---|
| 163–165 ↗ | (On Diff #18436) | If the only updated property is textCursorPositions then we're going to early exit from this method. We have to make sure that the test plan covers checking getDerivedStateFromProps method. |
Comment Actions
I'd say, if we save drafts, we should also save cursorPosition. It may be frustrating for user to edit two drafts at the same time, and have they cursor revert to the start of textarea every time. I'm not really sure how to estimate cost of keeping it, but if it isn't too high I'd keep it as nice to have feature.
On entering new thread we position cursor as it was left, so if user was editing some text, then jumped to another thread, replied to some messages there and went back to the original thread where he was editing some long message, their cursor will stay in the same place.
Comment Actions
@atul should take a look since this touches InputStateContainer
Putting it in context provides us with extra benefit of saving cursor position when jumping between different chats and channels.
Makes sense to me
| web/input/input-state.js | ||
|---|---|---|
| 38 ↗ | (On Diff #18477) | All of this should probably be $ReadOnly (eg. prefix each property with +) but not sure if that will introduce Flow errors |
Comment Actions
Changed context properties to read-only.
| web/input/input-state.js | ||
|---|---|---|
| 38 ↗ | (On Diff #18477) | Makes sense, I changed it and it haven't invoked any flow errors, so I'm amending it. |
Comment Actions
This looks reasonable to me.
(Did we take a look at any prior work for this? This seems like the sort of problem that others have probably run into before and there might be established or simpler solutions we could work off of?)
| web/input/input-state-container.react.js | ||
|---|---|---|
| 488 ↗ | (On Diff #18510) | Should we be defaulting to 0 here? What if the cursor isn't in the text? Shouldn't we instead set things to -1 or null so we can handle that case separately? Feel free to correct me if I'm wrong/missing context |
Comment Actions
(Did we take a look at any prior work for this? This seems like the sort of problem that others have probably run into before and there might be established or simpler solutions we could work off of?)
@atul I'm not sure what "this" refers to in your sentence (so ironic in JS review thread :D) Can you clarify what problem you're referring to? If you meant saving cursor position it was only reasonable solution I could think of. If we save draft texts in this manner, I thought it is okay to keep cursor positions in those drafts as well. And I need that that to save cursor position between renders of ChatInputBar.
| web/input/input-state-container.react.js | ||
|---|---|---|
| 488 ↗ | (On Diff #18510) | In future diffs, I'll set cursor in textarea to value from this context, so if it wasn't set yet, 0 is good default value meaning start of the text. Cursor position is only ever relevant when we're editing text. If textarea isn't focused it doesn't really matter |
Comment Actions
Do we still need to save cursor positions in a hypothetical solution where we use contenteditable = true?
Comment Actions
I think saving caret position is not strictly necessary now. I preferred to use it, as we treat textarea as controlled component, so we want to control it via state we can update use.
Without saving cursor position:
- We would need to rely on selection start and end from textarea element. With editable div, we would rely on global Selection object.
- We wouldn't get that nice-to-have saving cursor position when jumping between threads
Comment Actions
We wouldn't get that nice-to-have saving cursor position when jumping between threads
This part still applies for contenteditable = true?
Comment Actions
Yes, ff we swap textarea to contentEditable and keep saving caret position in context we keep the feature of saving its position when changing threads. It would require some changes, but nothing to serious.