Skip to content

Fixed "Message being edited is cancelled on pressing any key"…#6367

Merged
tgolen merged 6 commits into
Expensify:mainfrom
kursat:message-edit
Dec 1, 2021
Merged

Fixed "Message being edited is cancelled on pressing any key"…#6367
tgolen merged 6 commits into
Expensify:mainfrom
kursat:message-edit

Conversation

@kursat

@kursat kursat commented Nov 19, 2021

Copy link
Copy Markdown
Contributor

While editing a message, setting input to empty string was causing edit box disappear.

Details

  1. ReportActionItemMessageEdit component is rendering when draft is not an empty string. So we saved original message as a draft when user delete everything from the textarea. This fixed main problem.
  2. debouncedSaveDraft was not getting last changes. Changed function to use its parameter, removed last parameter of binding of _.debounce to debouncedSaveDraft. So it takes last changes now.
  3. Cancelled debouncedSaveDraft, it was causing another bug when you type and press enter in 1s (debounce duration).

Fixed Issues

$ #6155

Tests | QA Steps

  1. Right click on a comment.
  2. Click edit comment.
  3. Remove all text.
  4. Wait more than 1 second.
  5. Be sure textarea is not disappearing.

Tested On

  • Web
  • Mobile Web
  • Android

Screenshots

Web

2021-11-19.18-09-44.mp4

Mobile Web

2021-11-20.18-07-05.mp4

Android

2021-11-20.17-12-42.mp4

@github-actions

github-actions Bot commented Nov 19, 2021

Copy link
Copy Markdown
Contributor

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

@kursat

kursat commented Nov 19, 2021

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@kursat kursat marked this pull request as ready for review November 19, 2021 15:15
@kursat kursat requested a review from a team as a code owner November 19, 2021 15:15
@MelvinBot MelvinBot requested review from tgolen and removed request for a team November 19, 2021 15:15
@kursat kursat changed the title Fixed "Message being edited is cancelled on pressing any key - #6144"… Fixed "Message being edited is cancelled on pressing any key"… Nov 19, 2021

@tgolen tgolen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, just requesting a few contextual comments.

Comment thread src/pages/home/report/ReportActionItemMessageEdit.js
Comment thread src/pages/home/report/ReportActionItemMessageEdit.js

@tgolen tgolen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! They look pretty good. I just added one suggestion to make it a little more clear.

Can you also please do all the comments as inline // comments? We usually only do block /**/ comments for method docs or proptype docs

Comment thread src/pages/home/report/ReportActionItemMessageEdit.js Outdated
kursat and others added 2 commits November 19, 2021 23:25
Co-authored-by: Tim Golen <tgolen@gmail.com>

@parasharrajat parasharrajat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good work @kursat.
Few requests:

  1. Please test it on all platforms and Add a video for all of them which were mentioned on the template.
  2. Add Tests and QA steps, we can't move forward without those.

@kursat

kursat commented Nov 20, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @parasharrajat. I'll add test and q/a steps asap.

About the videos, I recorded one for android and I can record one for mobile web but since I don't have access a mac I'll not be able to record ios and desktop videos. Will be this 3 videos enough?

@parasharrajat

Copy link
Copy Markdown
Member

Thanks @kursat .

For other question, it's not my call. Although I think it's fine.

@kursat

kursat commented Nov 20, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the enlightenment @parasharrajat, @tgolen is this ok?

Thanks @parasharrajat. I'll add test and q/a steps asap.

About the videos, I recorded one for android and I can record one for mobile web but since I don't have access a mac I'll not be able to record ios and desktop videos. Will be this 3 videos enough?

@kursat kursat requested a review from tgolen November 23, 2021 18:24
@kursat

kursat commented Nov 29, 2021

Copy link
Copy Markdown
Contributor Author

Hey @tgolen, this PR is not merged for a while, is there something wrong?

@tgolen tgolen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay while I was out on vacation.

I don't think we've come to a good conclusion about developers that aren't able to build iOS or desktop. I think in this case, it seems like a pretty safe change that shouldn't have too much platform specific problems, so I think it's OK.

In other cases where there might be something platform specific, reaching out to the slack channel to see if someone can test out the branch and record it for you might be a good route to try.

@kursat

kursat commented Dec 1, 2021

Copy link
Copy Markdown
Contributor Author

Hey, no problem and sorry to bother you. I hope you had a good vacation :)

It's my first PR, and I am trying to figure out how things work. So I am a little nervous. Should I do something to merge this PR?

That's a great idea, I'll do that next time.

In other cases where there might be something platform specific, reaching out to the slack channel to see if someone can test out the branch and record it for you might be a good route to try.

@tgolen

tgolen commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

Should I do something to merge this PR?

Ah, no. That's my bad. I was waiting for @parasharrajat to approve because I saw he did a partial review and when there are multiple reviewers, it's good to wait until everyone has approved. However, I think Rajat was really just doing a process-check and not really a code review, so I will merge this.

@tgolen tgolen merged commit cef3c25 into Expensify:main Dec 1, 2021
@OSBotify

OSBotify commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

@kursat, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

  1. Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
  2. Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
  3. Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

@OSBotify

OSBotify commented Dec 1, 2021

Copy link
Copy Markdown
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.

@OSBotify

OSBotify commented Dec 7, 2021

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @tgolen in version: 1.1.17-8 🚀

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

@OSBotify

OSBotify commented Dec 8, 2021

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants