Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: paste and Ctrl+z #4131

Merged
merged 3 commits into from
Jul 19, 2021
Merged

fix: paste and Ctrl+z #4131

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7a1837
fix: paste and Ctrl+z
parasharrajat Jul 17, 2021
96f4086
Merge branch 'main' of github.com:Expensify/Expensify.cash into paste…
parasharrajat Jul 17, 2021
87b17dc
remove extra selection
parasharrajat Jul 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 6 additions & 24 deletions src/components/TextInputFocusable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ class TextInputFocusable extends React.Component {
end: initialValue.length,
},
};
this.selection = {
start: initialValue.length,
end: initialValue.length,
};
this.saveSelection = this.saveSelection.bind(this);
this.dragNDropListener = this.dragNDropListener.bind(this);
this.handlePaste = this.handlePaste.bind(this);
this.handlePastedHTML = this.handlePastedHTML.bind(this);
Expand Down Expand Up @@ -232,17 +227,6 @@ class TextInputFocusable extends React.Component {
}
}

/**
* Keeps track of user cursor position on the Composer
*
* @param {{nativeEvent: {selection: any}}} event
* @memberof TextInputFocusable
*/
saveSelection(event) {
this.selection = event.nativeEvent.selection;
this.props.onSelectionChange(event);
}

/**
* Manually place the pasted HTML into Composer
*
Expand All @@ -252,13 +236,11 @@ class TextInputFocusable extends React.Component {
handlePastedHTML(html) {
const parser = new ExpensiMark();
const markdownText = parser.htmlToMarkdown(html);
const beforeCursorText = this.textInput.value.substring(0, this.selection.start);
const afterCursorText = this.textInput.value.substring(this.selection.end);
this.textInput.value = beforeCursorText + markdownText + afterCursorText;
const newCursorPosition = beforeCursorText.length + markdownText.length;
this.setState({selection: {start: newCursorPosition, end: newCursorPosition}});
this.updateNumberOfLines();
this.props.onChangeText(this.textInput.value);
try {
document.execCommand('insertText', false, markdownText);
this.updateNumberOfLines();
// eslint-disable-next-line no-empty
} catch (e) {}
Comment on lines +239 to +243
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The execCommand API is deprecated. Is there an alternative to this?

Also, could you add a comment to explain why do we need an empty catch {} block here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HorusGoul Its deprecated but I didn't find any direct alternative. If we don't use it then we have to implement our own framework to manage undo redo and other operations. There are few WYSIWYG editors out there which does this but we are not currently using it.

After a careful inspection I come to the decision of using it, previously I was doing the manual paste but it lacked Undo functionality..

This is still supported for almost all browsers https://caniuse.com/?search=execCommand.
Also, I checked RN-WEB to see how they manage Copy to clipboard. They are also using it https://github.com/necolas/react-native-web/blob/98dcefd9041c10cc95a2385202498b274d926a6b/packages/react-native-web/src/exports/Clipboard/index.js#L50

I just borrowed the catch block from RN-web.

We can think about of changing to different alternative when we redesign our composer but I would say ATM its the best bet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Let's merge then :)

}

/**
Expand Down Expand Up @@ -353,7 +335,7 @@ class TextInputFocusable extends React.Component {
onChange={() => {
this.updateNumberOfLines();
}}
onSelectionChange={this.saveSelection}
onSelectionChange={this.onSelectionChange}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be this.props.onSelectionChange? Because onSelectionChange does not exist as a method on Composer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component is deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually while investigating Composer, I noticed that it has no this.onSelectionChange method, but it is being used in the component:

App/src/components/Composer/index.js

Lines 358 to 360 in 7b8f085

onChange={this.shouldCallUpdateNumberOfLines}
onSelectionChange={this.onSelectionChange}
numberOfLines={this.state.numberOfLines}

And found this PR, while checking out the git blame for it.
Could you please confirm that this.onSelectionChange should be changed to this.props.onSelectionChange in Composer/index.js?

numberOfLines={this.state.numberOfLines}
style={propStyles}
/* eslint-disable-next-line react/jsx-props-no-spreading */
Expand Down