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.

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

fix: paste and Ctrl+z #4131

Merged
merged 3 commits into from
Jul 19, 2021
Merged

fix: paste and Ctrl+z #4131

merged 3 commits into from
Jul 19, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jul 17, 2021

Details

Fixed Issues

$ Fixes #4120

Tests | QA Steps

  1. Log in to e.cash and navigate to a conversation.
  2. Send a message.
  3. After the message has been sent, copy it.
  4. In the compose box, enter some text.
  5. With the cursor at the end of the input, paste the message.
  6. Press the shortcut for undo (CTRL+Z).

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

paste.mp4

Mobile Web

Desktop

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner July 17, 2021 15:35
@MelvinBot MelvinBot requested review from HorusGoul and removed request for a team July 17, 2021 15:36
@mountiny mountiny self-requested a review July 19, 2021 10:26
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

@parasharrajat Tested based on instruction in here #3790.

I assume that copy pasting bold text from a message in E.cash is different problem, right?

@parasharrajat
Copy link
Member Author

Yes @mountiny That is a different issue.

Comment on lines +239 to +243
try {
document.execCommand('insertText', false, markdownText);
this.updateNumberOfLines();
// eslint-disable-next-line no-empty
} catch (e) {}
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 :)

@HorusGoul HorusGoul merged commit f9d0058 into Expensify:main Jul 19, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions bot pushed a commit that referenced this pull request Jul 20, 2021
fix: paste and Ctrl+z
(cherry picked from commit f9d0058)
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-2🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@isagoico
Copy link

This is working as intended in MacOS Desktop and Web only. Windows is still facing this same issue #4066

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@parasharrajat
Copy link
Member Author

@isagoico #4066 is different from thr problem this PR solves. It is meant to solve #4120.

I will look into that seperately.

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@@ -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.

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:

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants