Add Context menu with Pin option to LHN #16079

Gonals · 2023-03-17T17:31:02Z

Details

Surrounding the existing TouchableOpacity with a PressableWithSecondaryInteraction component did not work on mobile, as the triggered touch event was the TouchableOpacity one, so I replaced the TouchableOpacity with the PressableWithSecondaryInteraction and gave it opacity capabilities. All working nice now!

Fixed Issues

$ #15960

Tests

Log into NewDot
On the LHN, right click/long press on top of an unpinned chat.
A menu with the pin option should show up. Press it
Confirm it gets pinned
Do the same for a pinned chat and confirm you can unpin it
Open newdot in a different devide and log into the same account.
On Devide 1 right click/long press on top of a chat to display the menu. Leave it open
On Devide 2, pin/unpin the chat for which you open the menu on Device 1.
When the pin status update id Device 1, confirm the menu is automatically closed.
Open a chat and right click/long press one of the messages. Confirm the usual menu shows up as usual and it doesn't have the pin/unpin option.
On that menu, select the "Delete" option and confirm the message is correctly deleted
(On Web/Desktop only) Go to Settings > About > App Download links and right click on one of the links. A menu should show up with just the option to copy the link.

Verify that no errors appear in the JS console

Offline tests

Not relevant

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

MelvinBot · 2023-03-17T19:32:48Z

@cead22 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Gonals · 2023-03-22T15:44:17Z

OFF WIP! All yours @cead22

cead22 · 2023-03-23T19:01:11Z

I revived this slack discussion because I'm not convinced this is a good change

cead22

I think we this PR should have the following tests, but I'm curious for your thoughts

All uses of showContextMenu
All uses of PressableWithSecondaryInteraction
Deleting a report and make sure nothing breaks since hideDeleteModal changed

src/components/LHNOptionsList/OptionRowLHN.js

0xmiros · 2023-04-03T19:57:08Z

reviewing now

Gonals · 2023-04-04T14:49:33Z

I think we this PR should have the following tests, but I'm curious for your thoughts

All uses of showContextMenu

These changes shouldn't have affected other menus, right? We can add a test to just make sure the new "Pin/Unpin" doesn't show for other menus, but I think it'd be enough to change in any other use of the menu, instead of all of them

All uses of PressableWithSecondaryInteraction

Yep. This was a bigger one. Adding tests.

Deleting a report and make sure nothing breaks since hideDeleteModal changed

Where did it change? 😕

Gonals · 2023-06-01T07:32:47Z

Conflicts solved AGAIN!

0xmiros

👍

OSBotify · 2023-06-01T16:07:46Z

✋ 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 · 2023-06-02T12:41:01Z

🚀 Deployed to staging by https://github.com/cead22 in version: 1.3.23-0 🚀

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

OSBotify · 2023-06-02T13:20:32Z

🚀 Deployed to staging by https://github.com/cead22 in version: 1.3.23-0 🚀

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

0xmiros · 2023-06-04T19:56:27Z

src/components/LHNOptionsList/OptionRowLHN.js

+
+    React.useEffect(() => {
+        ReportActionContextMenu.hideContextMenu(false);
+    }, [optionItem.isPinned]);


@Gonals we should add null safety check on this line. Sometimes app crashes when logout because optionItem is undefined returning here:

App/src/libs/SidebarUtils.js

Lines 197 to 202 in 0813e2c

// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for

// this method to be called after the Onyx data has been cleared out. In that case, it's fine to do

// a null check here and return early.

if (!report || !personalDetails) {

return;

}

I faced this today

OSBotify · 2023-06-05T19:06:02Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.23-7 🚀

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

OSBotify · 2023-06-05T19:30:38Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.23-7 🚀

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

roryabraham · 2023-06-08T05:57:16Z

src/components/LHNOptionsList/OptionRowLHN.js

@@ -52,11 +55,16 @@ const defaultProps = {

 const OptionRowLHN = (props) => {
    const optionItem = SidebarUtils.getOptionData(props.reportID);
+
+    React.useEffect(() => {


I'm actually not seeing why this is necessary. I removed this and everything seems to work fine.

That discussion started from here.
Finally landed on #16079 (comment)

roryabraham · 2023-06-08T06:50:27Z

I think that using PopoverMenu may have been a more appropriate component than ReportActionContextMenu, which is specifically for ... report actions and their context menus.

parasharrajat · 2023-07-06T18:37:18Z

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

+    {
+        textTranslateKey: 'common.unPin',
+        icon: Expensicons.Pin,
+        shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat) => type === CONTEXT_MENU_TYPES.REPORT && isPinnedChat,


Pin/UnPin options should not be visible on MoneyRequest chats as they are not pinnable. #20255

abdulrahuman5196 · 2023-07-22T13:49:26Z

Minor miss in implementation - Pin/Unpin should have made the composer in focus - #21861

parasharrajat · 2023-07-22T19:22:50Z

@abdulrahuman5196 how will this work on Mobile devices. So I don't agree that these unpin/pin should focus on the composer even on web.

sobitneupane · 2023-08-09T07:50:52Z

This case was not considered while implementing the context menu in Sidebar.

Issue: Context menu from LHN is visible even if report page is opened on resize

Steps to reproduce:

Right click an option in LHN.
Resize the window to small while the popover is still open.

Gonals added 9 commits March 15, 2023 13:06

changing branchs

fc9aec9

Merge branch 'main' into alberto-pin

5bbf1db

add PIN action

ffaf634

correctly show menu

8789f75

handle pin/unpin behavior

d3befa3

no need fur success messages

28079fe

figure out what's going on

c3c45ce

make pin/unpin work fine

a6e1aec

cleanup

6b26871

Gonals requested a review from a team as a code owner March 17, 2023 19:32

melvin-bot bot requested review from cead22 and removed request for a team March 17, 2023 19:32

Gonals added 8 commits March 17, 2023 15:39

lint

cc83d47

more lint cleanup

6d65fb8

update test

96c8256

handle mobile devices

eafa87f

Merge branch 'main' into alberto-pin

310ae83

Use pressablewithSecondaryInteraction instead of touchableOpacity

315fbc6

Turn Pressable into touchableOpacity

7ef10b8

cleanUp

5803c47

Gonals changed the title ~~[WIP] Add Context menu with Pin option to LHN~~ Add Context menu with Pin option to LHN Mar 22, 2023

update test

9f4349d

cead22 reviewed Apr 3, 2023

View reviewed changes

src/components/LHNOptionsList/OptionRowLHN.js Outdated Show resolved Hide resolved

Gonals added 2 commits April 4, 2023 16:44

Merge branch 'main' into alberto-pin

233ae47

add better defaults

223a550

Gonals dismissed 0xmiros’s stale review via 137631e June 1, 2023 07:32

prettier

a433efb

0xmiros approved these changes Jun 1, 2023

View reviewed changes

cead22 approved these changes Jun 1, 2023

View reviewed changes

cead22 merged commit bcf92bb into main Jun 1, 2023

cead22 deleted the alberto-pin branch June 1, 2023 16:02

Ollyws mentioned this pull request Jun 1, 2023

Fix for: IOU threads don't have pin options #19851

Merged

57 tasks

OSBotify mentioned this pull request Jun 2, 2023

Deploy Checklist: New Expensify 2023-06-02 #20042

Closed

32 tasks

0xmiros reviewed Jun 4, 2023

View reviewed changes

This was referenced Jun 5, 2023

[HOLD for payment 2023-06-13] [HOLD for payment 2023-06-12] Add null check for optionItem #20154

Closed

Add null check before react effect #20155

Merged

melvin-bot bot mentioned this pull request Jun 5, 2023

[HOLD for payment 2023-06-12] Allow pinning from the chat list #15960

Closed

Skalakid mentioned this pull request Jun 6, 2023

[Group 2][Accessible Pressable] Migrate OptionRowLHN #17014

Closed

roryabraham reviewed Jun 8, 2023

View reviewed changes

0xmiros mentioned this pull request Jun 12, 2023

17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

Merged

57 tasks

parasharrajat reviewed Jul 6, 2023

View reviewed changes

parasharrajat mentioned this pull request Jul 6, 2023

[HOLD for payment 2023-07-03] [$1000] IOU has pin option in LHN #20255

Closed

6 tasks

mollfpr mentioned this pull request Jul 9, 2023

[HOLD for payment 2023-07-10] [$1000] Report List option is not highlighted on long pressing in LNH #21066

Closed

6 tasks

abdulrahuman5196 mentioned this pull request Jul 22, 2023

[HOLD for checklist] [$1000] Web - Pressing the Pin/Unpin option from the LHN context menu does not refocus the composer. #21861

Closed

6 tasks

sobitneupane mentioned this pull request Aug 9, 2023

[HOLD for payment 2023-08-08] [$1000] [HOLD] Context menu from LHN is visible even if report page is opened on resize #21929

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Context menu with Pin option to LHN #16079

Add Context menu with Pin option to LHN #16079

Gonals commented Mar 17, 2023 •

edited

Loading

MelvinBot commented Mar 17, 2023

Gonals commented Mar 22, 2023

cead22 commented Mar 23, 2023

cead22 left a comment

0xmiros commented Apr 3, 2023

Gonals commented Apr 4, 2023 •

edited

Loading

Gonals commented Jun 1, 2023

0xmiros left a comment

OSBotify commented Jun 1, 2023

OSBotify commented Jun 2, 2023

OSBotify commented Jun 2, 2023

0xmiros Jun 4, 2023

fedirjh Jun 5, 2023

OSBotify commented Jun 5, 2023

OSBotify commented Jun 5, 2023

roryabraham Jun 8, 2023

0xmiros Jun 8, 2023

roryabraham commented Jun 8, 2023

parasharrajat Jul 6, 2023

abdulrahuman5196 commented Jul 22, 2023

parasharrajat commented Jul 22, 2023 •

edited

Loading

sobitneupane commented Aug 9, 2023

	// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for
	// this method to be called after the Onyx data has been cleared out. In that case, it's fine to do
	// a null check here and return early.
	if (!report \|\| !personalDetails) {
	return;
	}

Add Context menu with Pin option to LHN #16079

Add Context menu with Pin option to LHN #16079

Conversation

Gonals commented Mar 17, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Mar 17, 2023

Gonals commented Mar 22, 2023

cead22 commented Mar 23, 2023

cead22 left a comment

Choose a reason for hiding this comment

0xmiros commented Apr 3, 2023

Gonals commented Apr 4, 2023 • edited Loading

Gonals commented Jun 1, 2023

0xmiros left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 1, 2023

OSBotify commented Jun 2, 2023

OSBotify commented Jun 2, 2023

0xmiros Jun 4, 2023

Choose a reason for hiding this comment

fedirjh Jun 5, 2023

Choose a reason for hiding this comment

OSBotify commented Jun 5, 2023

OSBotify commented Jun 5, 2023

roryabraham Jun 8, 2023

Choose a reason for hiding this comment

0xmiros Jun 8, 2023

Choose a reason for hiding this comment

roryabraham commented Jun 8, 2023

parasharrajat Jul 6, 2023

Choose a reason for hiding this comment

abdulrahuman5196 commented Jul 22, 2023

parasharrajat commented Jul 22, 2023 • edited Loading

sobitneupane commented Aug 9, 2023

Gonals commented Mar 17, 2023 •

edited

Loading

Gonals commented Apr 4, 2023 •

edited

Loading

parasharrajat commented Jul 22, 2023 •

edited

Loading