#2821 add up arrow to edit feature #3157

dklymenk · 2021-05-26T07:00:18Z

Details

In addition to solution described in my original proposal, code in this PR should be faster because it doesn't loop over all messages to filter the ones where user is the author, but instead it loops over the list in reverse order and stops on the first matching result.

Please note that this feature is not present on native apps, the same way the shortcut for sending message (Enter/Return key) is not working on native apps.

Fixed Issues

Fixes #2821

Tests / QA Steps

Web and Desktop:

Open a chat where you already have at least one message sent. (Or write a message if you don't have one)
While message box is focused, press ArrowUp.
The edit box for your last message should appear.

Android and iOS:

Since the feature is not present on native, make sure basic chatting functionality is not broken.

Tested On

Screenshots

Web

2021-05-26.09-57-07.mp4

Mobile Web

Simulator.Screen.Recording.-.iPhone.11.-.2021-05-26.at.10.27.42.mp4

Desktop

Screen.Recording.2021-05-26.at.10.33.13.mov

iOS

Android

roryabraham

Nice, this is looking great and testing well!

src/pages/home/report/ReportActionCompose.js

roryabraham · 2021-05-26T19:05:48Z

All yours @thienlnam

parasharrajat · 2021-05-26T20:20:36Z

src/pages/home/report/ReportActionCompose.js

+                    Object.keys(this.props.reportActions).reverse(),
+                    key => this.props.reportActions[key].actorEmail === this.props.session.email,


Not sure if this the correct way to do it. You should sort by the sequenceNumber.

I can see that report actions are being sorted by sequenceNumber here:
https://github.com/Expensify/Expensify.cash/blob/9f305176681385f2aa80f43de06fc6dffa7f0b83/src/pages/home/report/ReportActionsView.js#L243-L255

Is this a requirement, however? In my local storage for any ReportActions_<report_id> sequenceNumber always matches the key of a report action.

What do you mean? This code just loops though the reportActions backwards and finds the first one where the actor email is the current user.

Objects keys are not sequential. so keys' order could be discrete.

All the keys in my localstorage are ordered and identical to sequenceNumber. If one can't trust this order, I think running sortBy on entire object for every action trigger would not be very efficient. It would be better to have some storage for sorted list shared with ReportActionsView.js in that case.

parasharrajat · 2021-05-26T20:35:54Z

@roryabraham I noticed one thing may be not related to this issue. We can see the edit action on IOU actions but not edit box open for them and this PR will try to follow the same when user will press the UP Arrow.

roryabraham · 2021-05-26T20:53:31Z

@parasharrajat Thanks for reviewing! Good point about not filtering out IOU actions!

Check out these lines from the DeleteComment pull request. It implements a function (reused for edit comment and delete comment) called canEdit that determines if the user should be able to edit that action.

So what we should do is:

Also return false in that function if the reportAction is an IOU action
Move that function to a separate library so it can be imported and used here.

dklymenk · 2021-05-26T21:12:28Z

Ok, I see.

I do the check for reportActionID, that's enough to avoid editing the newly created message, but I see the need to have a separate function for that since it's such a widely used check.

So I create a new file src/libs/canEdit.js with default export? Or add a function to some existing one?

Also since that PR is not merged yet, I can't really "move" the function to separate library, so i'll just add a function and won't touch the original implementation.

parasharrajat · 2021-05-26T21:17:36Z

I think reportUtils.js will be a good place to use and you can suggest that function on the Delete comment PR.

roryabraham · 2021-05-26T21:50:09Z

Agreed, in this PR we can just add a function to reportUtils.js called canEditReportAction, then import it and use it both in the ReportActionContextMenu and here in the ReportActionCompose

dklymenk · 2021-05-26T22:10:45Z

Updated the PR with change regarding the new function.

I also took the liberty of adding the isReportMessageAttachment call to the new canEditReportAction function as well.

roryabraham

Two other suggestions:

I noticed that you didn't import & use your new function in ReportActionContextMenu here. Let's do that to DRY this up.
Instead of using withOnyx and passing in the sessionEmail as a parameter to the canEditReportAction function, you could instead use Onyx.connect directly in reportUtils to keep a local reference to the current user's email. That way we have one Onyx subscription to the SESSION key instead of 2+

roryabraham · 2021-05-26T22:34:42Z

src/libs/reportUtils.js

+function canEditReportAction(reportAction, sessionEmail) {
+    return reportAction.actorEmail === sessionEmail
+        && reportAction.reportActionID
+        && reportAction.actionName !== 'IOU'


Suggested change

&& reportAction.actionName !== 'IOU'

&& reportAction.actionName === 'ADDCOMMENT'

Also, you could optionally add a const to CONST.js called REPORT_ACTION.ACTION_TYPES so that we're not using a string literal here.

roryabraham · 2021-05-26T22:41:34Z

Here's an example of what I mean by using Onyx.connect directly in the library function instead of using withOnyx in the React code.

…se Onyx.connect directly in reportUtils + add CONST.REPORT_ACTION + check for ADDCOMMENT

dklymenk · 2021-05-28T16:38:51Z

I've applied requested changes. @roryabraham, please check. Thank you.

roryabraham · 2021-05-28T17:34:22Z

src/CONST.js

@@ -47,6 +47,7 @@ const CONST = {
        TYPE: {
            CHAT: 'chat',
            IOU: 'iou',
+            ADDCOMMENT: 'ADDCOMMENT',


You've created REPORT.TYPE.ADDCOMMENT here, but we want REPORT.ACTION.TYPE.ADDCOMMENT

sorry, fixed

dklymenk · 2021-05-28T18:20:47Z

Fixed 2 merge conflicts. Please make sure this change is ok with you:

https://github.com/Expensify/Expensify.cash/pull/3157/files#diff-ab84993378320f81d528552bc52c215aca2f62eb7aa0036c0329a1fcde8911f6L124-R114

roryabraham

This was broken after your last merge. To fix it, I think you'll need to:

Remove the canEdit member function from ReportActionContextMenu (including from the constructor)
Replace it's current usage in the shouldShow for deleteComment with () => canEditReportAction(this.props.reportAction)

Please be careful to retest locally before each new request for review, especially if you had to resolve merge conflicts.

src/libs/reportUtils.js

roryabraham · 2021-05-29T15:34:35Z

src/pages/home/report/ReportActionContextMenu.js

-                    this.canEdit()
-                    && !isReportMessageAttachment(this.getActionText())
-                ),
+                shouldShow: canEditReportAction(this.props.reportAction),


Suggested change

shouldShow: canEditReportAction(this.props.reportAction),

shouldShow: () => canEditReportAction(this.props.reportAction),

dklymenk · 2021-05-29T17:00:48Z

My bad on this one. For some reason I did not think that conflict resolution would break stuff.

Applied your suggestions. Thanks.

roryabraham · 2021-05-31T17:17:50Z

@dklymenk Sorry, there are more merge conflicts now 😞

dklymenk · 2021-05-31T17:25:09Z

@roryabraham

Resolved the conflict. Tested. Everything should be ok.

roryabraham · 2021-05-31T17:41:18Z

looks good and tests well 👍

roryabraham · 2021-05-31T17:42:25Z

@thienlnam All yours for the merge after E2E tests are done 👍

roryabraham · 2021-06-01T14:52:24Z

Since Jack approved a previous version of this pull request, and I think this might resolve this deploy blocker, I'm going to go ahead and merge this.

isagoico · 2021-06-01T20:17:20Z

@dklymenk We found this issue in mWeb

mWeb - Up arrow not enabled for edit feature

Expected Result

The edit box for your last message should appear.

Actual Result

Arrow up is disabled and unable to select by tapping

Action Performed

Launch the url and login
Open a chat where you already have at least one message sent. (Or write a message if you don't have one)
While message box is focused, press ArrowUp.

Platform

mWeb ✔️

Notes/images/video

Bug5094951_Up_arrow.mp4

roryabraham · 2021-06-01T20:27:38Z

No need to block deploys on this @isagoico, but @dklymenk let's get this fixed with a follow-up PR. You showed a video of it working, so this hopefully should be pretty simple.

dklymenk · 2021-06-02T06:20:40Z

@isagoico Those "arrow keys" on your video are grayed out(inactive,disabled). Tapping them doesn't provide any feedback to user nor does it send any events to browser.

Moreover, those are not actually arrow keys. A quick online search told me, that they behave more like Tab and Shit+Tab to jump between inputs on a form.
Here is a demo (note that buttons become enabled when there is an action available):

Simulator_Screen_Recording_iPhone_11_2021_06_02_at_08_33_10.mp4

I used https://www.w3schools.com/html/tryit.asp?filename=tryhtml_form_submit, but any form with proper html should do the trick.

Now I don't have a real iPhone, and to my understanding you cannot install apps from app store on a simulator, so if you can install some third-party full 5 row keyboard from store, try with that and it won't work, I'll try to get my hands on one to fix the issue.

@roryabraham The video I showed for mobile web is from ~~Chromium dev tools responsive mode~~ iPhone simulator and I pressed an Up Arrow on my actual laptop keyboard there, but to demonstrate that it actually works on real phone, I have shot a few videos: one with third-party keyboard from F-Droid and one with actual physical keyboard connected via usb-c dongle:

video_2021-06-02_09-11-59.mp4

test.mp4

Hopefully, this clears up the confusion and the issue can be closed again. Thanks.

EDIT: I meant "the HOLD for payment can be added back", and not "the issue can be closed again".

EDIT2: Corrected the origin of demo mWeb video in the first post of the PR.

iwiznia · 2021-06-02T18:20:52Z

I agree with @dklymenk

roryabraham · 2021-06-02T19:05:01Z

Thanks @dklymenk!

dklymenk added 5 commits May 25, 2021 19:32

Expensify#2821 add edit last message shortcut

13aa539

Expensify#2821 add mac meta key check

051a4fd

Expensify#2821 remove ctrl/cmd key checks

cc4d5e7

Expensify#2821 add check for reportActionID

caf5732

Expensify#2821 fix comment

754c702

dklymenk marked this pull request as ready for review May 26, 2021 07:40

dklymenk requested a review from a team as a code owner May 26, 2021 07:40

MelvinBot requested review from thienlnam and removed request for a team May 26, 2021 07:41

roryabraham self-requested a review May 26, 2021 16:28

roryabraham assigned roryabraham and dklymenk and unassigned roryabraham May 26, 2021

roryabraham previously approved these changes May 26, 2021

View reviewed changes

src/pages/home/report/ReportActionCompose.js Outdated Show resolved Hide resolved

parasharrajat suggested changes May 26, 2021

View reviewed changes

Expensify#2821 rename triggerShortcut to triggerHotkeyActions

f7d336b

dklymenk dismissed roryabraham’s stale review via f7d336b May 26, 2021 20:38

dklymenk and others added 3 commits May 27, 2021 00:57

Expensify#2821 create canEditReportAction function in libs

83ed684

Expensify#2821 add jsDocs for canEditReportAction function

6e6ab1a

Merge branch 'main' into 2821-add-up-arrow-to-edit-feature

2c9ea91

roryabraham requested changes May 26, 2021

View reviewed changes

Expensify#2821 use canEditReportAction in ReportActionContextMenu + u…

70a3ff0

…se Onyx.connect directly in reportUtils + add CONST.REPORT_ACTION + check for ADDCOMMENT

Expensify#2821 remove withOnyx from ReportActionContextMenu

162aca1

dklymenk dismissed thienlnam’s stale review via 162aca1 May 28, 2021 16:33

dklymenk requested a review from roryabraham May 28, 2021 16:39

roryabraham requested changes May 28, 2021

View reviewed changes

Expensify#2821 change const path

68361f2

dklymenk requested a review from roryabraham May 28, 2021 17:50

Merge branch 'main' into 2821-add-up-arrow-to-edit-feature

8eb04d2

roryabraham requested changes May 29, 2021

View reviewed changes

roryabraham mentioned this pull request May 29, 2021

iOS - App is extremely slow to load conversations #3167

Closed

Expensify#2821 fix merge conflict issues

fb72c89

dklymenk requested a review from roryabraham May 29, 2021 17:01

Merge branch 'main' into 2821-add-up-arrow-to-edit-feature

0ac381d

roryabraham approved these changes May 31, 2021

View reviewed changes

roryabraham merged commit 9dfcd8e into Expensify:main Jun 1, 2021

OSBotify mentioned this pull request Jun 1, 2021

Deploy Checklist: Expensify.cash 2021-05-28 #3199

Closed

27 tasks

roryabraham mentioned this pull request Jun 1, 2021

Delete comment - Deleted comment is still visible in receiver side #3249

Closed

roryabraham mentioned this pull request Jun 1, 2021

[Hold for Payment on 8 June] When in the compose box, allow clicking the up-arrow key to edit the previous message. #2821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2821 add up arrow to edit feature #3157

#2821 add up arrow to edit feature #3157

dklymenk commented May 26, 2021 •

edited by roryabraham

Loading

roryabraham left a comment

roryabraham commented May 26, 2021

parasharrajat May 26, 2021

dklymenk May 26, 2021

roryabraham May 26, 2021

parasharrajat May 26, 2021

dklymenk May 26, 2021

parasharrajat commented May 26, 2021

roryabraham commented May 26, 2021 •

edited

Loading

dklymenk commented May 26, 2021

parasharrajat commented May 26, 2021

roryabraham commented May 26, 2021 •

edited

Loading

dklymenk commented May 26, 2021

roryabraham left a comment

roryabraham May 26, 2021

roryabraham commented May 26, 2021 •

edited

Loading

dklymenk commented May 28, 2021

roryabraham May 28, 2021

dklymenk May 28, 2021

dklymenk commented May 28, 2021

roryabraham left a comment •

edited

Loading

roryabraham May 29, 2021

dklymenk commented May 29, 2021

roryabraham commented May 31, 2021

dklymenk commented May 31, 2021

roryabraham commented May 31, 2021

roryabraham commented May 31, 2021

roryabraham commented Jun 1, 2021

isagoico commented Jun 1, 2021

roryabraham commented Jun 1, 2021

dklymenk commented Jun 2, 2021 •

edited

Loading

iwiznia commented Jun 2, 2021

roryabraham commented Jun 2, 2021

		Object.keys(this.props.reportActions).reverse(),
		key => this.props.reportActions[key].actorEmail === this.props.session.email,

	&& reportAction.actionName !== 'IOU'
	&& reportAction.actionName === 'ADDCOMMENT'

	shouldShow: canEditReportAction(this.props.reportAction),
	shouldShow: () => canEditReportAction(this.props.reportAction),

#2821 add up arrow to edit feature #3157

#2821 add up arrow to edit feature #3157

Conversation

dklymenk commented May 26, 2021 • edited by roryabraham Loading

Details

Fixed Issues

Tests / QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented May 26, 2021

roryabraham commented May 26, 2021 • edited Loading

dklymenk commented May 26, 2021

parasharrajat commented May 26, 2021

roryabraham commented May 26, 2021 • edited Loading

dklymenk commented May 26, 2021

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented May 26, 2021 • edited Loading

dklymenk commented May 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklymenk commented May 28, 2021

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklymenk commented May 29, 2021

roryabraham commented May 31, 2021

dklymenk commented May 31, 2021

roryabraham commented May 31, 2021

roryabraham commented May 31, 2021

roryabraham commented Jun 1, 2021

isagoico commented Jun 1, 2021

mWeb - Up arrow not enabled for edit feature

Expected Result

Actual Result

Action Performed

Platform

roryabraham commented Jun 1, 2021

dklymenk commented Jun 2, 2021 • edited Loading

iwiznia commented Jun 2, 2021

roryabraham commented Jun 2, 2021

dklymenk commented May 26, 2021 •

edited by roryabraham

Loading

roryabraham commented May 26, 2021 •

edited

Loading

roryabraham commented May 26, 2021 •

edited

Loading

roryabraham commented May 26, 2021 •

edited

Loading

roryabraham left a comment •

edited

Loading

dklymenk commented Jun 2, 2021 •

edited

Loading