-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IS-3218] Fixed cursor issue on edit comment box #3434
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,12 +36,26 @@ class ReportActionItemMessageEdit extends React.Component { | |
this.debouncedSaveDraft = _.debounce(this.debouncedSaveDraft.bind(this), 1000, true); | ||
this.publishDraft = this.publishDraft.bind(this); | ||
this.triggerSaveOrCancel = this.triggerSaveOrCancel.bind(this); | ||
this.onSelectionChange = this.onSelectionChange.bind(this); | ||
|
||
this.state = { | ||
draft: this.props.draftMessage, | ||
selection: { | ||
start: this.props.draftMessage.length, | ||
end: this.props.draftMessage.length, | ||
}, | ||
}; | ||
} | ||
|
||
/** | ||
* Update Selection on change cursor position. | ||
* | ||
* @param {Event} e | ||
*/ | ||
onSelectionChange(e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall looks good but this state change is unnecessary, I have already seen a drawback of this and I am migrating tis unnecessary state change. So my suggestion here would be to remove this as I mentioned over the issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @parasharrajat can you please let me know about the issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is going against our old pattern of not making the Component controlled. |
||
this.setState({selection: e.nativeEvent.selection}); | ||
} | ||
|
||
/** | ||
* Update the value of the draft in Onyx | ||
* | ||
|
@@ -109,6 +123,8 @@ class ReportActionItemMessageEdit extends React.Component { | |
toggleReportActionComposeView(false); | ||
}} | ||
autoFocus | ||
selection={this.state.selection} | ||
onSelectionChange={this.onSelectionChange} | ||
/> | ||
</View> | ||
<View style={[styles.flexRow, styles.mt1]}> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting selection without focusing the input internally focuses the input on Safari which is then shifts to other elements causing bugs such as #15105