Edit Comments by yuwenmemon · Pull Request #2320 · Expensify/App

yuwenmemon · 2021-04-09T07:09:51Z

Pullerbearing (@MariaHCD)
cc @roryabraham

Details

Give users the ability to edit their comments!

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/147482
Fixes https://github.com/Expensify/Expensify/issues/162717

Tests/QA

Open up a chat
On Web/Desktop, click the pencil button that shows when you hover over a chat message of your own. On Mobile, long tap a message of your own
Make sure that the comment shows up inside of the text input with the "Cancel" and "Save" buttons underneath
Edit the comment, click "Cancel" make sure that the original comment still shows
Edit the comment, click "Save" make sure that the comment shows as what you've edited it to become, with (edited) showing up next to the comment now

Hover over a comment that is not your own, make sure that no "pencil" edit icon shows up and that you cannot edit that comment

Tested On

[ x ] Web
[ x ] Mobile Web
[ x ] Desktop
[ x ] iOS
Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

I can't get the Android emulator to run without slowing my machine to a crawl so animations, so here are some stills:

… hardcoded comment in code

roryabraham · 2021-04-15T01:20:07Z

+                    style={[styles.textInput, styles.flex0]}
+                />
+                <View style={[styles.flexRow, styles.mt1]}>
+                    <TouchableOpacity style={[styles.button, styles.mr2]}>


As stated in this thread, I would prefer to see something like this:

<Pressable onPress={this.deleteDraft} style={({hovered, pressed}) => [ styles.button, styles.mr2, getButtonBackgroundColorStyle(getButtonState(hovered, pressed)), ]} />

Also, I see you switched to Pressable but didn't use the style callback w/ getButtonBackgroundColorStyle(getButtonState(hovered, pressed))

We might need @Expensify/design's help to think around 2 things:

The normal button hover color is the same as the hover color for the comment:

There is a greenHover color, however, what should we use for greenPressed?

Or perhaps better yet, do we need button styles for hover for the edit comment buttons?

roryabraham · 2021-04-15T01:23:29Z

+                            Cancel
+                        </Text>
+                    </TouchableOpacity>
+                    <TouchableOpacity style={[styles.button, styles.buttonSuccess]}>


Same here, but you would just have to create something like getButtonSuccessBackgroundColorStyle

<Pressable onPress={this.deleteDraft} style={({hovered, pressed}) => [ styles.button, styles.mr2, getButtonSuccessBackgroundColorStyle(getButtonState(hovered, pressed)), ]} />

roryabraham · 2021-04-15T01:23:48Z

+                        </Text>
+                    </TouchableOpacity>
+                    <TouchableOpacity style={[styles.button, styles.buttonSuccess]}>
+                        <Text style={[styles.buttonText, styles.buttonSuccessText]} onPress={this.publishDraft}>


Same here with the onPress given to the Text

roryabraham · 2021-04-15T23:19:34Z

    fetchAll(false);
 });

+function editReportComment(reportID, reportAction, htmlForNewComment) {


Missing a method doc here

roryabraham · 2021-04-15T23:32:12Z


+function editReportComment(reportID, reportAction, htmlForNewComment) {
+    // Optimistically update the report action with the new message
+    const sequenceNumber = reportAction.sequenceNumber;


I was just reviewing @sketchydroide's delete comment PR, and I thought the way he approached this syntactically was a bit cleaner (and has better error handling):

function editReportComment(reportID, reportAction, htmlForNewComment) { // Optimistically update Onyx actionToMerge = {}' const oldMessage = {...reportAction.message}; actionToMerge[reportAction.sequenceNumber] = { ...reportAction, message: [ { type: 'COMMENT', html: htmlForNewComment, text: htmlForNewComment.replace(/<[^>]*>?/gm, ''), }, ], }; Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge); // Make the API request API.Report_EditComment({ reportID, reportActionID: reportAction.reportActionID, reportComment: htmlForNewComment, }) .catch(() => { // If it fails, reset Onyx actionToMerge[reportAction.sequenceNumber] = { ...reportAction, message: oldMessage, }; Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, reportActionsToMerge); }); }

I like the error handling, but I actually find my way to be less confusing. Maybe that's just because I wrote it so I'm familiar with it, but it's a little more clear what's exactly is being edited.

yuwenmemon · 2021-04-30T23:51:29Z

HEYO! I am removing this WIP! Please take a look! 🙇

OSBotify · 2021-05-11T01:12:23Z

🚀 Deployed to staging in version: 1.0.41-7🚀

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

isagoico · 2021-05-11T20:11:07Z

Blue screen displayed when trying to edit message.

Expected Result

A context menu should show up

Actual Result

Screen becomes blue after long tap on message

Action Performed

Open https://staging.expensify.cash/
Login with existing account
Open any chat
Long tap a message of your own

Platform

iOS ✔️

Notes/images/Video

Bug5063742_RPReplay_Final1620758969.mp4

roryabraham · 2021-05-11T20:28:31Z

@isagoico This could happen before this PR, can you create a separate issue?

isagoico · 2021-05-11T20:34:22Z

Tester was unable to reproduce the same blue screen in prod but it seems really similar to this issue #2705. Should I include the new info there? or just create a separate issue?

roryabraham · 2021-05-11T20:39:54Z

Okay, yeah good idea to just add it in there

isagoico · 2021-05-11T20:47:52Z

Perfect, will remove the deploy blocker label then

OSBotify · 2021-05-13T23:05:15Z

🚀 Deployed to production in version: 1.0.44-0🚀

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

mananjadhav · 2023-04-27T09:46:43Z

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

s77rt · 2023-09-21T14:53:29Z

+     */
+    deleteDraft() {
+        saveReportActionDraft(this.props.reportID, this.props.action.reportActionID, '');
+        toggleReportActionComposeView(true, this.props.isSmallScreenWidth);


This caused a regression (case 2). If we delete any comment the main composer will show up, even if we are deleting a non-focused message.

What we had to do is to add a condition where we show the main composer only if we are deleting the currently focused message.

ntdiary · 2024-04-10T13:10:59Z

+        super(props);
+        this.updateDraft = this.updateDraft.bind(this);
+        this.deleteDraft = this.deleteDraft.bind(this);
+        this.debouncedSaveDraft = _.debounce(this.debouncedSaveDraft.bind(this), 1000, true);


Coming from #38483.
We can clear the side effects like this debounce when the component unmounts.
No one could have imagined that there would be multiple instances of this component at the same time. 😂

rojiphil · 2024-06-24T03:48:14Z

+                    style={[styles.textInput, styles.flex0]}
+                    onFocus={() => {
+                        scrollToIndex({animated: true, index: this.props.index}, true);
+                        toggleReportActionComposeView(false);


Since scrolling and toggling of the composer could result in a race condition due to animations, it is better to wait for the animations to complete before scrolling. This caused an issue #40767 where the composer box is partially hidden.

yuwenmemon added 5 commits March 22, 2021 15:30

Edit Comments - fundamental API portions, version allows to edit to a…

b318a69

… hardcoded comment in code

Get dirty editing UI in place

41e885d

Fix conflicts?

8f1fb48

reall fix conflicts

94053ea

Really non-performant comment editing

bf1e59c

yuwenmemon requested a review from a team as a code owner April 9, 2021 07:09

MelvinBot requested review from MariaHCD and removed request for a team April 9, 2021 07:10

yuwenmemon added 7 commits April 13, 2021 15:10

Make sure we have reportID in grouped report action item

8500513

Fix the stylings for a message being edited

8f8fd1c

Make it so that clicking the edit comment button again cancels the edit

00fae5d

Add keypress handlers

e967c38

Fix conflicts

f94a9c4

Revert some unneccessary measureContent crap I edited out

cc295ae

Fix a couple bugs

235cf83

roryabraham reviewed Apr 15, 2021

View reviewed changes

Add edited indicator

40c7c30

roryabraham reviewed Apr 15, 2021

View reviewed changes

yuwenmemon added 4 commits April 23, 2021 15:24

Add some comments, get this in more a polished state, PR comments

ca5d13e

Fix conflicts

32103ae

hidePopover

5d0536f

Fix conflicts

2594d4f

yuwenmemon force-pushed the yuwen-editComments branch from 268af75 to 2594d4f Compare April 28, 2021 05:14

yuwenmemon added 4 commits April 28, 2021 10:13

Playing around with ReportScrollManager

ce406cd

Scroll touch screens to comment when editing a comment

a29f272

Show/hide compose box when focusing on editing comment in mobile

da33bc6

Linting

1ef1bce

yuwenmemon changed the title ~~[WIP] Edit Comments~~ Edit Comments Apr 30, 2021

yuwenmemon merged commit 66186d1 into main May 11, 2021

yuwenmemon deleted the yuwen-editComments branch May 11, 2021 00:43

OSBotify mentioned this pull request May 11, 2021

Deploy Checklist: Expensify.cash 2021-05-08 #2751

Closed

36 tasks done

twisterdotcom mentioned this pull request May 11, 2021

[Pay 5/19] Wrap text when it includes markup #2790

Closed

3 of 5 tasks

yuwenmemon mentioned this pull request May 11, 2021

Make sure withLocalize forwards its ref #2800

Merged

1 of 5 tasks

isagoico added the DeployBlockerCash This issue or pull request should block deployment label May 11, 2021

isagoico mentioned this pull request May 11, 2021

mWeb -Chat - Weird behavior with 3D Touch on chat screen #2705

Closed

isagoico removed the DeployBlockerCash This issue or pull request should block deployment label May 11, 2021

NikkiWines mentioned this pull request May 18, 2021

Edit button does not show up on Hover Menu for recently sent message #2966

Closed

Jag96 mentioned this pull request May 26, 2021

iOS - App is extremely slow to load conversations #3167

Closed

eVoloshchak mentioned this pull request Mar 8, 2023

Scroll To Last Index After Layout Change Of List #15662

Closed

54 of 55 tasks

alexxxwork mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683

Closed

fedirjh mentioned this pull request Apr 27, 2023

[HOLD for payment 2023-04-27] [HOLD for payment 2023-04-26] [$1000] The edited message doesn't update on LHN in offline mode #17212

Closed

2 of 6 tasks

sobitneupane mentioned this pull request Jun 9, 2023

[HOLD for payment 2023-06-07] [$1000] The horizontal line (for deleted message) is not in the center for the text 'edited' but is center for the 'chat messages' on web chrome (offline mode) #17181

Closed

1 of 6 tasks

s77rt reviewed Sep 21, 2023

View reviewed changes

s77rt mentioned this pull request Sep 21, 2023

[HOLD for payment 2023-09-27] [$2000] Chat - Main composer is displayed when editing two messages #23908

Closed

2 of 6 tasks

ntdiary reviewed Apr 10, 2024

View reviewed changes

ntdiary mentioned this pull request Apr 10, 2024

[HOLD for payment 2024-04-09] [$500] Weird pop up when hit esc key immediately, after clicking uparrow to edit the last message #38483

Closed

1 of 6 tasks

rojiphil reviewed Jun 24, 2024

View reviewed changes

rojiphil mentioned this pull request Jun 24, 2024

[HOLD for payment 2024-07-10] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] Chat - Compose box is hidden when editing multiple line break message #40767

Closed

1 of 6 tasks

wildan-m mentioned this pull request Nov 29, 2024

[Due for payment 2025-02-13] [$250] mWeb/Chrome - Chat - Edition composer is cutted when editing multiline message near the top of screen #52138

Closed

1 of 8 tasks

MelvinBot mentioned this pull request Sep 23, 2026

Android - Compose box - Draft message input reappears after clearing and navigating #98674

Closed

1 of 7 tasks

Conversation

yuwenmemon commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests/QA

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwenmemon commented Apr 30, 2021

Uh oh!

OSBotify commented May 11, 2021

Uh oh!

isagoico commented May 11, 2021

Blue screen displayed when trying to edit message.

Expected Result

Actual Result

Action Performed

Platform

Uh oh!

roryabraham commented May 11, 2021

Uh oh!

isagoico commented May 11, 2021

Uh oh!

roryabraham commented May 11, 2021

Uh oh!

isagoico commented May 11, 2021

Uh oh!

OSBotify commented May 13, 2021

Uh oh!

mananjadhav commented Apr 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

yuwenmemon commented Apr 9, 2021 •

edited

Loading