Context-menu repositioned #4194

parasharrajat · 2021-07-23T18:37:57Z

Details

Fixed Issues

$ fixes #4106

Tests | QA Steps

Open E.cash.
Check that the context menu is active on all platforms.

Tested On

Screenshots

I will update screenshots after the first code review.

Web

context.mp4

Mobile Web

Desktop

iOS

Android

marcaaron · 2021-07-27T02:25:10Z

I guess this is me. I was hoping to have someone else give this an initial review. @roryabraham are you available to help ?

marcaaron · 2021-07-27T02:32:30Z

@parasharrajat Are you able to add some benchmarks to the description? 🙏
This is supposed to improve performance of chat switching. I am curious to see if it will.

parasharrajat · 2021-07-27T15:57:31Z

I am not sure on How exactly we record that? but I'll try. I saw that you posted some stats on the issue, I'll take that as a reference.

roryabraham · 2021-07-27T16:01:00Z

@marcaaron I can help review 😄

roryabraham · 2021-07-27T16:01:12Z

Also @parasharrajat conflicts

marcaaron · 2021-07-28T17:58:11Z

When the delete modal will show we have to update the state isDeleteModalVisible which will cause the re-render of the list. Also, DeleteModal is linked with ContextMenu which is handled well together. If we move this out, we have to pass more callbacks here and there.

Ok cool, let's skip my suggestion for now. It will add more complexity to what is already pretty complex changes.

marcaaron · 2021-07-28T19:57:45Z

Testing is going really well @parasharrajat

I have built the PR on Android in release mode and seeing a reduction in chat switching times of about 0.7 seconds.

The metrics on dev are probably inflated a bit as release builds are more efficient in general but that is still a HUGE improvement.

parasharrajat · 2021-07-28T20:03:38Z

Yeah. Release build will be faster.

marcaaron

Changes look good to me. I'm gonna test this on web and iOS next, but maybe @roryabraham can do a last review. Let's get this merged!!

marcaaron · 2021-07-28T20:02:44Z

src/components/PopoverWithMeasuredContent.js

+        if (!props.isVisible) {
+            return {isVisible: false};
+        }
+        return null;


Ok, we can maybe try to improve this in a follow up I don't have a better suggestion at the moment.

marcaaron · 2021-07-28T20:06:34Z

src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

+        });
+    }
+
+    isActiveReportAction(actionID) {


Missing JS doc

marcaaron · 2021-07-28T20:08:17Z

src/pages/home/report/ContextMenu/ReportActionContextMenu.js

+    if (!contextMenuRef.current) {
+        return;
+    }
+    return contextMenuRef.current.hideDeleteModal();


NAB, is there some reason we are returning this function?

Sorry, It must have been left unintentionally. Only isActiveReportAction needs return value. I will update it.

marcaaron · 2021-07-28T20:56:20Z

Savings on iOS release builds are less significant, but still really good with an averages of 233 ms in my most recent test.

roryabraham

Looking better, only a few minor NAB code style comments at this point.

roryabraham · 2021-07-28T21:21:18Z

src/pages/home/report/ContextMenu/ContextMenuActions.js

+import {hideContextMenu, showDeleteModal} from './ReportActionContextMenu';
+
+/**
+ * Gets the markdown version of the message in an action.


NAB:

Suggested change

* Gets the markdown version of the message in an action.

* Gets the HTML version of the message in an action.

roryabraham · 2021-07-28T21:24:18Z

src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

+     * @param {Number} reportID - Active Report Id
+     * @param {Object} reportAction - ReportAction for ContextMenu
+     * @param {String} draftMessage - ReportAction Draftmessage
+     * @param {Function} [onShow=() => {}] - Run a callback when Menu is shown


NAB this is weird:

Suggested change

* @param {Function} [onShow=() => {}] - Run a callback when Menu is shown

* @param {Function} [onShow] - Run a callback when Menu is shown

roryabraham · 2021-07-28T21:24:27Z

src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

+     * @param {Object} reportAction - ReportAction for ContextMenu
+     * @param {String} draftMessage - ReportAction Draftmessage
+     * @param {Function} [onShow=() => {}] - Run a callback when Menu is shown
+     * @param {Function} [onHide=() => {}] - Run a callback when Menu is hidden


Suggested change

* @param {Function} [onHide=() => {}] - Run a callback when Menu is hidden

* @param {Function} [onHide] - Run a callback when Menu is hidden

roryabraham · 2021-07-28T21:26:41Z

src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

+        });
+    }
+
+    runAfterContextMenuHide() {


NAB because both names are bad:

Suggested change

runAfterContextMenuHide() {

runAndResetOnPopoverHide() {

roryabraham · 2021-07-28T21:32:44Z

src/pages/home/report/ReportActionsView.js

+
+                    // We use a combination of sequenceNumber and clientID in case the clientID are the same - which
+                    // shouldn't happen, but might be possible in some rare cases.
+                    // eslint-disable-next-line react/jsx-props-no-multi-spaces


bump – reasonably certain we don't need this eslint-disable

roryabraham · 2021-07-28T21:34:46Z

src/components/PopoverWithMeasuredContent.js

+        if (!props.isVisible) {
+            return {isVisible: false};
+        }
+        return null;


Would be good to leave a code comment explaining this.

parasharrajat · 2021-07-28T21:43:52Z

// eslint-disable-next-line react/jsx-props-no-multi-spaces is needed due to the comment over the keyExtractor prop. What if I move this comment on the function definition itself. Will it be fine?

parasharrajat · 2021-07-28T22:32:05Z

Changes done. Please review.

OSBotify · 2021-07-28T23:39:26Z

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

marcaaron · 2021-07-29T02:05:23Z

There's a problem with this PR on web. We need to remove this code I think as the method was renamed maybe?

this.runAfterContextMenuHide = this.runAfterContextMenuHide.bind(this);

parasharrajat · 2021-07-29T05:51:38Z

#4297 for a fix.

There is one more thing that is throwing a warning. forwardedRef prop type on WithLocalize. I don't know what type to pass, Seems correct to me.

marcaaron · 2021-07-29T15:48:24Z

I am unsure myself, but also see the error..

OSBotify · 2021-07-30T20:10:44Z

🚀 Deployed to staging in version: 1.0.81-5🚀

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

OSBotify · 2021-08-06T02:44:37Z

🚀 Deployed to production in version: 1.0.82-7🚀

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

parasharrajat added 6 commits July 23, 2021 04:22

Context-menu repositioned

d869215

fix: state props

fd92bb3

fix: positioning of menu

a6e4b74

fixed ContextMenu actions

f9a0332

finalize the menu

2ef41c7

Optimizations for Conextmenu

8e8e221

parasharrajat changed the title ~~[WIP] Context-menu repositioned~~ Context-menu repositioned Jul 25, 2021

cleanup

7d883ae

parasharrajat marked this pull request as ready for review July 25, 2021 20:35

parasharrajat requested a review from a team as a code owner July 25, 2021 20:35

MelvinBot requested review from marcaaron and removed request for a team July 25, 2021 20:35

parasharrajat added 4 commits July 26, 2021 17:42

refactor Context Menu

e0451f6

Pull latest changes

70491bf

refactor

00478fe

fix: realtime update issue with menus

b1b9877

marcaaron requested a review from a team July 27, 2021 02:23

MelvinBot removed the request for review from a team July 27, 2021 02:23

marcaaron assigned parasharrajat Jul 27, 2021

marcaaron requested review from a team and removed request for marcaaron July 27, 2021 02:24

MelvinBot requested review from marcaaron and removed request for a team July 27, 2021 02:24

parasharrajat mentioned this pull request Jul 27, 2021

Andrew 3313 link context menu #3931

Merged

5 tasks

roryabraham self-requested a review July 27, 2021 16:01

break fix

baf1d29

parasharrajat force-pushed the context-menu branch from 1f141bf to baf1d29 Compare July 28, 2021 17:31

marcaaron previously approved these changes Jul 28, 2021

View reviewed changes

marcaaron requested a review from roryabraham July 28, 2021 21:13

roryabraham previously approved these changes Jul 28, 2021

View reviewed changes

refactor code and added comments

bec3d56

parasharrajat dismissed stale reviews from roryabraham and marcaaron via bec3d56 July 28, 2021 22:29

parasharrajat requested review from marcaaron and roryabraham July 28, 2021 22:32

marcaaron approved these changes Jul 28, 2021

View reviewed changes

roryabraham approved these changes Jul 28, 2021

View reviewed changes

marcaaron merged commit 0b6263b into Expensify:main Jul 28, 2021

parasharrajat mentioned this pull request Jul 29, 2021

[No QA] Fix crash #4297

Merged

mallenexpensify mentioned this pull request Jul 29, 2021

[Pay Aug 18 if no regressions] Copy to clipboard - Unable to copy images or links #3313

Closed

This was referenced Jul 29, 2021

Deploy Checklist: Expensify.cash 2021-07-28 #4279

Closed

Deploy Checklist: Expensify.cash 2021-07-30 #4333

Closed

ishpaul777 mentioned this pull request Nov 8, 2023

[$500] IOS - The app flashes after selecting 'Copy to Clipboard' #30706

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Context-menu repositioned #4194

Context-menu repositioned #4194

parasharrajat commented Jul 23, 2021 •

edited

Loading

marcaaron commented Jul 27, 2021

marcaaron commented Jul 27, 2021

parasharrajat commented Jul 27, 2021

roryabraham commented Jul 27, 2021

roryabraham commented Jul 27, 2021

marcaaron commented Jul 28, 2021

marcaaron commented Jul 28, 2021

parasharrajat commented Jul 28, 2021

marcaaron left a comment

marcaaron Jul 28, 2021

marcaaron Jul 28, 2021

marcaaron Jul 28, 2021

parasharrajat Jul 28, 2021

marcaaron commented Jul 28, 2021

roryabraham left a comment

roryabraham Jul 28, 2021

roryabraham Jul 28, 2021

roryabraham Jul 28, 2021

roryabraham Jul 28, 2021

roryabraham Jul 28, 2021

roryabraham Jul 28, 2021

parasharrajat commented Jul 28, 2021

parasharrajat commented Jul 28, 2021

OSBotify commented Jul 28, 2021

marcaaron commented Jul 29, 2021

parasharrajat commented Jul 29, 2021 •

edited

Loading

marcaaron commented Jul 29, 2021

OSBotify commented Jul 30, 2021

OSBotify commented Aug 6, 2021

	* Gets the markdown version of the message in an action.
	* Gets the HTML version of the message in an action.

	* @param {Function} [onShow=() => {}] - Run a callback when Menu is shown
	* @param {Function} [onShow] - Run a callback when Menu is shown

	* @param {Function} [onHide=() => {}] - Run a callback when Menu is hidden
	* @param {Function} [onHide] - Run a callback when Menu is hidden

Context-menu repositioned #4194

Context-menu repositioned #4194

Conversation

parasharrajat commented Jul 23, 2021 • edited Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Jul 27, 2021

marcaaron commented Jul 27, 2021

parasharrajat commented Jul 27, 2021

roryabraham commented Jul 27, 2021

roryabraham commented Jul 27, 2021

marcaaron commented Jul 28, 2021

marcaaron commented Jul 28, 2021

parasharrajat commented Jul 28, 2021

marcaaron left a 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

Choose a reason for hiding this comment

marcaaron commented Jul 28, 2021

roryabraham left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Jul 28, 2021

parasharrajat commented Jul 28, 2021

OSBotify commented Jul 28, 2021

marcaaron commented Jul 29, 2021

parasharrajat commented Jul 29, 2021 • edited Loading

marcaaron commented Jul 29, 2021

OSBotify commented Jul 30, 2021

OSBotify commented Aug 6, 2021

parasharrajat commented Jul 23, 2021 •

edited

Loading

parasharrajat commented Jul 29, 2021 •

edited

Loading