Andrew 3313 link context menu #3931

Drewfergusson · 2021-07-08T13:35:24Z

Details

In order to enable a context menu on secondary actions (right-clicking on desktops or long-pressing on mobile), I created a reusable component called PressbaleWithContextMenu that would wrap anything that you wanted to have a context menu. I then applied this to the tags in the RenderHTML component. PressableWithContextMenu can also be used when we go to add actions to images or any other element.

I did copy a lot of functions and logic from the existing ReportActionItem. There wasn't a great way to reuse that code. Since we're now using that same popover logic in more places the logic needed to be in its own isolated component. I could have used mixins or inherited those functions but it would have created a dependency. I think as an optimization the ReportActionItems should implement the reusable PressableWithContextMenu as well.

Fixed Issues

#3313

Tests

Create a message containing a valid URL (ex https://expensify.com)
Send the message
Once the message is sent, notice that the link/URL has turned blue
Long-press on the blue link (or right-click)
You should be prompted with the option "Copy URL to Clipboard", click this option
You should now have that URL in your clipboard as a simple string.
Paste the URL into any pastable location (notepad, another message, etc)

QA Steps

Tested On

Screenshots

iOS

mobile-test.mov

Web

Mobile Web

Desktop

Android

github-actions · 2021-07-08T13:35:43Z

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

Drewfergusson · 2021-07-08T13:42:01Z

Still working on testing, just creating this PR as a placeholder

roryabraham · 2021-07-08T17:40:32Z

Okay, @Drewfergusson, just ping me when this is ready for review.

roryabraham · 2021-07-08T17:41:29Z

Also, just FYI for the future, you can always create a draft PR if your PR is not yet ready for review. 👍

Drewfergusson · 2021-07-09T22:47:30Z

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

Drewfergusson · 2021-07-18T20:15:14Z

@roryabraham @puneetlath this PR is ready for review. Testing evidence is attached.

roryabraham · 2021-07-27T00:22:46Z

Sorry for the delay @Drewfergusson, I was out-of-office last week and should be able to review this soon. However, it seems that there is now a merge conflict you'll need to resolve.

parasharrajat · 2021-07-27T08:55:19Z

To add more to the context, I think we should hold here as there is ongoing changes of refactoring the ContextMenu. #4194.

Drewfergusson · 2021-07-28T23:02:31Z

I'm glad @parasharrajat commented because, yes I did resolve another merge conflict and I saw @parasharrajat was moving the context menu for more general use.

@roryabraham, as he said, I may have to wait until he is finished.

roryabraham · 2021-07-29T00:12:39Z

@Drewfergusson #4194 is merged, so feel free to resolve conflicts and start working on this again when you're ready.

Drewfergusson · 2021-08-01T18:01:44Z

@roryabraham merge conflicts resolved and did a quick sanity test on web and iOS to ensure the features are still working. Please review when you have a chance

roryabraham

Great work so far!

src/languages/en.js

src/components/PressableWithContextMenu.js

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.native.js

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.js

roryabraham

Meant to hit "request changes" 🤦

parasharrajat

I am probably late to the party but We migrated our ContextMenu code to the Singleton component. and the same should be followed here. Although changes are awesome, you are using nested ReportActionContext which is something that we have moved away from.

@roryabraham Do you think a refactor is needed? It's going to be small changes you just have to call methods from the ReportActionContextMenu to show and hide the menu.

src/components/RenderHTML/BaseRenderHTML.js

parasharrajat · 2021-08-01T18:24:02Z

src/components/AnchorForCommentsOnly/anchorContextMenuOptions.js

+import {Clipboard as ClipboardIcon, Checkmark} from '../Icon/Expensicons';
+
+
+export default function anchorContextMenuOptions(href, translate) {


It's better to add this action to ContextMenuAction and manage the list of actions on the context menu by manipulating the array on the main PopoverReportActionContextmenu.

this action's definition will never change. Will it?

I think that the list of context actions will need to be dynamic depending on the context. So by default it will be the existing menu, but if we longPress on a link, we'll show "Copy URL to Clipboard".

So yeah I think we're on the same page here 🙃

side note: ContextMenuActions.js should be renamed contextMenuActions.js because it doesn't export a React component

yeah. My idea was to use the shouldShow flag on actions and manage the actions on the context menu. So we just need to pass the keys to actions. Something like that.

parasharrajat · 2021-08-03T14:54:32Z

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.js

-    >
-        {children}
-    </Text>
+    <PressableWithContextMenu contextMenuItems={anchorContextMenuOptions(href, translate)}>


Just Wrap this Anchor in PressableWithSecondaryInteraction
and Call the methods from ReportActionContextMenu.js.

roryabraham · 2021-08-03T14:55:26Z

@Drewfergusson another concern here is performance. We recently merged a PR that elevated the existing PopoverReportActionContextMenu to be a sibling of the main FlatList used for a chat. In so doing, we removed a huge amount of duplicate rendering logic from individual ReportActionItem components, and got a big performance boost.

Since this PR adds a new popover menu for each link in a chat, I'm pretty sure it will cause a substantial performance regression. To avoid this, I think we need to continue to reuse the single PopoverReportActionContextMenu per chat. A few quick notes to get you started:

This ReportActionContextMenu file is actually not a component, but a set of functions that can be used to control the main PopoverReportActionContextMenu. (it should really be camelCase instead of PascalCase)
We might not really need PressableWithContextMenu anymore. Instead, I think we just need be able to dynamically provide different context actions to the main PopoverReportActionContextMenu. You could probably do that by moving your anchor context actions into here.

roryabraham · 2021-08-03T14:56:58Z

hehe @parasharrajat I was too slow to send my last comment before you got a review in 😏

parasharrajat · 2021-08-03T15:02:03Z

Actually, I was just leaving a trail for you to catch up. So eventually you had to write all that stuff. 😉. @roryabraham.

Drewfergusson · 2021-08-04T16:36:44Z

Hey @parasharrajat, just took a look at your changes. The single context menu is a huge improvement ✨ . I will work to implement copying links by controlling this menu. The popover currently does seem tightly coupled to report actions (it requires you to pass in a reportAction object or it breaks everything). So I will have to make it function for other use cases. I will work to push a PR soon. @roryabraham

parasharrajat · 2021-08-04T16:46:37Z

@Drewfergusson I think you can pass the default values for extra keys.
Look at the state to see what is the default and you have disable extra actions which are not needed.

Using your logic, you can pass in the actions. in your case, Copy the link to clipboard, and Open link.
and Configure contextMenu to take dynamic actions. this should work.

roryabraham

Overall approach looks great! It's a much simpler PR than before, which is good. Went ahead and requested a few changes with the current implementation.

src/components/PressableWithSecondaryInteraction/index.js

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

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

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

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.native.js

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

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.native.js

parasharrajat · 2021-08-05T15:43:48Z

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.native.js

+                    linkRef.current,
+                );
+            }
+        }
        onPress={() => (shouldDownloadFile ? fileDownload(href) : Linking.openURL(href))}


I think, It should have an action on the contextMenu as well. Open Link which can follow what you are doing now.

Not sure if the click to open feature is yet planned. cc: @roryabraham.

@roryabraham can you clarify @parasharrajat 's comment here? Doesn't seem like a bad idea to add Open Link to the context menu however in my testing a single/normal press opens the link. Not sure the exact scenario where they would get a download instead of going to the link.

Ah, I missed this. Yeah, I don't think we really need Open Link in the context menu, but since you've already added it we can leave it.

Let's ask in #expensify-open-source

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

Drewfergusson · 2021-08-08T22:00:39Z

@roryabraham @parasharrajat I have updated the code based on your feedback. I even went ahead and added the 'Open Link' option to the context menu as suggested by @parasharrajat. It's working on desktop and iOS simulator. If you approve the code I will add all other test evidence.

Would love to get this one across the finish line :)

roryabraham

Looking good now. Just a few minor code style changes and lint errors that need to be resolved.

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.js

src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.native.js

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

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

Drewfergusson · 2021-08-09T19:31:26Z

@roryabraham changes made and testing evidence/screenshots attached.

roryabraham

Looks like some change here broke the mini ReportActionContextMenu that should appear on hover:

roryabraham

Just a few more things:

We can get rid of this context action. It never shows and is useless.
I figured out why the mini ReportActionContextMenu isn't displayed. Here's how to fix it (in BaseReportActionContextMenu):

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

Drewfergusson · 2021-08-11T03:17:44Z

@roryabraham I removed the copy link option (as well as the associated translations and imports). I also applied your fix to the mini context menu. I confirmed it's working. Good catch, I didn't even notice that feature until you pointed it out.

OSBotify · 2021-08-11T21:24:22Z

✋ 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 · 2021-08-17T18:53:51Z

🚀 Deployed to staging by @roryabraham in version: 1.0.85-10 🚀

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

isagoico · 2021-08-18T15:50:11Z

@Drewfergusson Hello! We're only able to see the Copy to clipboard option. The Open link option from the screenshots is not displayed, is this expected? It's a bit confusing since the QA steps doesn't mention it but the screenshots show it.

roryabraham · 2021-08-18T22:44:15Z

Hi @isagoico, we removed the Open Link option that was displayed in the screenshots – what you've described is the expected behavior. Sorry for the confusion!

botify · 2021-08-25T17:33:59Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-01. 🎊

OSBotify · 2021-08-25T17:56:35Z

🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀

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

botify · 2021-08-30T17:21:11Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-06. 🎊

Pluto0104 · 2023-09-14T00:35:20Z

src/components/PressableWithSecondaryInteraction/index.js

@@ -30,6 +30,7 @@ class PressableWithSecondaryInteraction extends Component {
     */
    executeSecondaryInteractionOnContextMenu(e) {
        const selection = window.getSelection().toString();
+        e.stopPropagation();


Hi, why do we need this line? It seems to be unnecessary. cc @roryabraham @parasharrajat @puneetlath @Drewfergusson

This prevents event bubbling when this component is nested. We do not want nested and parent component both to trigger the action.

This happens for nested links in the message. When link context menu is triggered, message context menu should not override it.

Ah, so you mean by this case?

Drewfergusson requested a review from a team as a code owner July 8, 2021 13:35

MelvinBot requested review from puneetlath and removed request for a team July 8, 2021 13:35

roryabraham self-requested a review July 8, 2021 17:40

Drewfergusson mentioned this pull request Jul 12, 2021

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

Closed

roryabraham reviewed Aug 3, 2021

View reviewed changes

roryabraham self-requested a review August 3, 2021 14:11

roryabraham requested changes Aug 3, 2021

View reviewed changes

parasharrajat suggested changes Aug 3, 2021

View reviewed changes

changes to translation

f3c920a

Drewfergusson added 4 commits August 4, 2021 15:58

integrating links with popover singleton

88573be

adding type prop to context menu

96f939d

adding show/hide logic to context menu items based on type

15dcd92

passing context menu type through to modal functions

def27fb

roryabraham requested changes Aug 5, 2021

View reviewed changes

parasharrajat suggested changes Aug 5, 2021

View reviewed changes

Drewfergusson added 3 commits August 8, 2021 16:29

PR review changes made

6a33d8b

text copy change

bb5ea42

adding openLink choice

d4b896c

resolving mrege conflicts

9bbd5b3

roryabraham requested changes Aug 9, 2021

View reviewed changes

linting/code-review changes

2bdfe69

roryabraham requested changes Aug 9, 2021

View reviewed changes

roryabraham requested changes Aug 11, 2021

View reviewed changes

src/pages/home/report/ContextMenu/ContextMenuActions.js Outdated Show resolved Hide resolved

Drewfergusson added 3 commits August 10, 2021 21:54

removing copy link context menu action

25e1a00

removing dependencies related to opening links

0b17d56

fixing mini report action context menu

9cb6058

roryabraham approved these changes Aug 11, 2021

View reviewed changes

roryabraham merged commit b858fff into Expensify:main Aug 11, 2021

OSBotify mentioned this pull request Aug 12, 2021

Deploy Checklist: Expensify.cash 2021-08-11 #4554

Closed

53 tasks

roryabraham mentioned this pull request Aug 12, 2021

[$250] [Image] Implement the ability to copy images to the clipboard #4629

Closed

OSBotify mentioned this pull request Aug 17, 2021

Deploy Checklist: New Expensify 2021-08-17 #4701

Closed

parasharrajat mentioned this pull request Sep 18, 2021

[HOLD for payment 2021-12-13] URL link is mangled up with the rest of the text #4911

Closed

Pluto0104 reviewed Sep 14, 2023

View reviewed changes

		import {Clipboard as ClipboardIcon, Checkmark} from '../Icon/Expensicons';


		export default function anchorContextMenuOptions(href, translate) {

Andrew 3313 link context menu #3931

Andrew 3313 link context menu #3931

Conversation

Drewfergusson commented Jul 8, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

iOS

Web

Mobile Web

Desktop

Android

github-actions bot commented Jul 8, 2021 • edited Loading

Drewfergusson commented Jul 8, 2021

roryabraham commented Jul 8, 2021

roryabraham commented Jul 8, 2021

Drewfergusson commented Jul 9, 2021

Drewfergusson commented Jul 18, 2021

roryabraham commented Jul 27, 2021

parasharrajat commented Jul 27, 2021

Drewfergusson commented Jul 28, 2021

roryabraham commented Jul 29, 2021

Drewfergusson commented Aug 1, 2021

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat 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

roryabraham commented Aug 3, 2021

roryabraham commented Aug 3, 2021

parasharrajat commented Aug 3, 2021

Drewfergusson commented Aug 4, 2021 • edited Loading

parasharrajat commented Aug 4, 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

Drewfergusson commented Aug 8, 2021

roryabraham left a comment

Choose a reason for hiding this comment

Drewfergusson commented Aug 9, 2021

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham left a comment

Choose a reason for hiding this comment

Drewfergusson commented Aug 11, 2021

OSBotify commented Aug 11, 2021

OSBotify commented Aug 17, 2021

isagoico commented Aug 18, 2021

roryabraham commented Aug 18, 2021

botify commented Aug 25, 2021

OSBotify commented Aug 25, 2021

botify commented Aug 30, 2021

Choose a reason for hiding this comment

parasharrajat Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drewfergusson commented Jul 8, 2021 •

edited

Loading

github-actions bot commented Jul 8, 2021 •

edited

Loading

roryabraham left a comment •

edited

Loading

Drewfergusson commented Aug 4, 2021 •

edited

Loading

parasharrajat Sep 14, 2023 •

edited

Loading