Add Expensify.cash to iOS and Android native share menus #1361

r8 · 2021-01-29T00:08:52Z

Details

Uses react-native-share-menu to add Expensify.cash to the native share menu on mobile platforms and enables sharing to the app.

Chat selection is made on separate page, which reuses components from Home page. To achieve this I had to modify some sidebar related components and functions, mainly to add callbacks for getting selected chat id.

Fixed Issues

Fixes #1294

Tests

Open some other app on iOS or Android, for example image gallery or web browser.
Activate share menu for the element that needs to be shared.
Confirm that Expensify.cash is present in Share menu.
Select Expensify.cash.
Confirm that Expensify.cash is opened and routes to Send to... page.
Select or create and select some chat.
Confirm that shared item is posted to selected chat.

Tested On

Screenshots

iOS

ShareMenu-iOS.mp4

Android

ShareMenu-Android.mp4

github-actions · 2021-01-29T00:09:14Z

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

TomatoToaster · 2021-02-02T16:54:15Z

Great job on this! I gave this a passthrough and it looks good but it seems like there might be some unintended differences as a result of a merge conflict? Could you give it a look and resolve the merge conflicts before I give it a full review?

r8 · 2021-02-02T16:57:25Z

@TomatoToaster Oh, sorry. It seems that something has been merged after I've submitted the PR, and I haven't noticed it until now.
I'll resolve the conflicts shortly.

TomatoToaster · 2021-02-02T17:23:06Z

No problem at all, Thanks!

r8 · 2021-02-02T22:09:53Z

@TomatoToaster Conflicts are resolved

TomatoToaster

Hey sorry, this took a bit for me to review, but this mostly looks good! I had a few adjustments requested. LMK what you think.

src/pages/home/sidebar/SidebarLinks.js

src/pages/SharePage.js

TomatoToaster · 2021-02-04T05:14:18Z

ios/Podfile.lock


-COCOAPODS: 1.10.0
+COCOAPODS: 1.10.1


Would you mind reverting to 1.10.0 to build this project? We haven't switched to 1.10.1 quite yet and it would help keep our mobile repos in sync.

I managed to do it by reinstalling the cocoapods gem to that version and deleting this file before a pod install.

I had to use 1.10.1, because you have this version in your Gemfile.lock

https://github.com/Expensify/Expensify.cash/blob/d353a46e95efd3e9d0df85134e078aa0e859b8fa/Gemfile.lock#L34

I had 1.10.0 too, but pod install wasn't working and was showing following error: Could not find proper version of cocoapods (1.10.1) in any of the sources.

So I'm not sure how to proceed. I can revert to 1.10.0, but are you sure that it will work?

Ah I see, I didn't realize it was locked to that version in the gemfile. Let me double check on that.

TomatoToaster

This looks pretty good to me, let me double check about the gemfile situation.

marcaaron

Have not tested this but just leaving some thoughts...

Probably we should be have used the new OptionsList instead of ChatSwitcherView for this as the latter will be deleted soon. The adaptation of the SidebarLinks for use in SharePage could benefit from some clean up. Seems like there is a lot of duplication which will make this code hard to maintain.

marcaaron · 2021-02-04T21:52:40Z

src/libs/ShareManager/index.native.js

+ * @param {Object} sharedItem
+ */
+function handleShare(sharedItem) {
+    if (sharedItem) {


This should be an early return

marcaaron · 2021-02-04T21:53:59Z

src/libs/ShareManager/index.native.js

+ * Removes share events listener.
+ */
+function deregister() {
+    listener.remove();


Should we set the listener back to undefined? Probably we should not call remove() if it doesn't exist. Have seen this happen with some other listeners.

marcaaron · 2021-02-04T21:55:46Z

src/pages/SharePage.js

+                                onClose={this.toggleCreateMenu}
+                                isVisible={this.state.isCreateMenuActive}
+                                onItemSelected={this.onCreateMenuItemSelected}
+                            />


We probably don't need the CreateMenu here?

Same with the FAB?

marcaaron · 2021-02-04T21:58:32Z

src/pages/home/sidebar/ChatSwitcherView.js

@@ -241,7 +245,8 @@ class ChatSwitcherView extends React.Component {
     */
    startGroupChat() {
        const userLogins = _.map(this.state.usersToStartGroupReportWith, option => option.login);
-        fetchOrCreateChatReport([this.props.session.email, ...userLogins]);
+        fetchOrCreateChatReport([this.props.session.email, ...userLogins])
+            .then(this.props.onReportSelected);


I'm confused about why we added this. And if this is what we want why did we not add a .then() to fetchOrCreateChatReport() instead adding it to several usages?

marcaaron · 2021-02-04T22:01:23Z

src/libs/actions/Report.js

@@ -504,6 +504,7 @@ function fetchAll(shouldRedirectToReport = true, shouldFetchActions = false, sho
 * set of participants
 *
 * @param {String[]} participants
+ * @returns {Promise} reportID


Why should this return a Promise?

marcaaron · 2021-02-04T22:08:11Z

src/pages/SharePage.js

+        redirect(this.props.currentlyViewedReportID !== ''
+            ? ROUTES.getReportRoute(this.props.currentlyViewedReportID)
+            : ROUTES.HOME);
+    }


Seems like duplicated redirect logic here. Maybe could have abstracted this somehow.

marcaaron · 2021-02-04T22:10:27Z

src/styles/styles.js

+        paddingLeft: 20,
+        paddingRight: 0,
+        paddingVertical: 12,
+    },


Rather than create a new style we should use padding spacing helpers.

marcaaron · 2021-02-04T22:11:07Z

src/pages/home/sidebar/SidebarLinks.js

+                                    onPress={this.props.onCloseButtonClick}
+                                >
+                                    <Icon src={Close} />
+                                </TouchableOpacity>


This could probably be refactored into a CloseButton component.

marcaaron · 2021-02-04T22:15:24Z

src/pages/home/sidebar/SidebarLinks.js

-                                    isActive={this.props.network && !this.props.network.isOffline}
-                                />
-                            </TouchableOpacity>
+                            {this.props.showCloseButton && (


If we need to add things like showCloseButton and showAvatar then it might make more sense to reduce the responsibility of these components and use a version that allows adding in the header parts where they are needed instead of a new prop to make optional.

marcaaron · 2021-02-04T22:18:29Z

src/pages/home/sidebar/ChatSwitcherView.js

        } else {
-            fetchOrCreateChatReport([this.props.session.email, selectedOption.login]);
+            fetchOrCreateChatReport([this.props.session.email, selectedOption.login])
+                .then(this.props.onReportSelected);


Just FYI this entire file will be removed very soon and this change makes it difficult to do.

TomatoToaster · 2021-02-05T01:08:25Z

ios/ExpensifyCash.xcodeproj/project.pbxproj

+				MARKETING_VERSION = 1.0.1;
+				MTL_ENABLE_DEBUG_INFO = INCLUDE_SOURCE;
+				MTL_FAST_MATH = YES;
+				PRODUCT_BUNDLE_IDENTIFIER = com.chat.expensify.chat.ShareExtension;


Hey also as a note, there might need to be more done with adding this bundle. The logs for our iOS deploy shows there was an error with this identifier.

In the new PR we can figure out fixes for this and what needs to be done.

r8 added 11 commits January 27, 2021 20:29

Install share menu dependencies

8ad5a46

Configure share menu for android

1daf456

Configure share menu for iOS

8c0d3d4

Add Share Manager library

88a8180

Implement inital structure for Share page

5135fa2

Add ability to redefine Chat Links title

23b7dc5

Move shared item storage operations to actions

0f485ce

Implement close button for Share Page

cd8b1ee

Implement callbacks for Share Page components

0c183a6

Implement callback for selected report on Shared Page

3be8773

Post shared item to selected report

52d0c56

r8 added 4 commits January 29, 2021 02:42

Add mock for ShareManager so loginTest does not fail

ea4dea1

Fix ShareExtension target configuration

090e25c

Accept any file on Android

06f24df

Create proper data object for shared file

4e963df

r8 marked this pull request as ready for review January 31, 2021 22:34

r8 requested a review from a team as a code owner January 31, 2021 22:34

r8 changed the title ~~[WIP] Add Expensify.cash to iOS and Android native share menus~~ Add Expensify.cash to iOS and Android native share menus Jan 31, 2021

botify requested review from TomatoToaster and removed request for a team January 31, 2021 22:35

Merge branch 'master' into sergey-share-menu

9478373

TomatoToaster suggested changes Feb 4, 2021

View reviewed changes

Change default value for showAvatar to avoid implicit behaviour

f7429a6

TomatoToaster approved these changes Feb 4, 2021

View reviewed changes

TomatoToaster merged commit 0e371a5 into Expensify:master Feb 4, 2021

github-actions bot locked and limited conversation to collaborators Feb 4, 2021

marcaaron reviewed Feb 4, 2021

View reviewed changes

TomatoToaster reviewed Feb 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Expensify.cash to iOS and Android native share menus #1361

Add Expensify.cash to iOS and Android native share menus #1361

r8 commented Jan 29, 2021 •

edited

Loading

github-actions bot commented Jan 29, 2021 •

edited

Loading

TomatoToaster commented Feb 2, 2021

r8 commented Feb 2, 2021

TomatoToaster commented Feb 2, 2021

r8 commented Feb 2, 2021

TomatoToaster left a comment

TomatoToaster Feb 4, 2021

r8 Feb 4, 2021

TomatoToaster Feb 4, 2021

TomatoToaster left a comment

marcaaron left a comment

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

marcaaron Feb 4, 2021

TomatoToaster Feb 5, 2021


		COCOAPODS: 1.10.0
		COCOAPODS: 1.10.1

Add Expensify.cash to iOS and Android native share menus #1361

Add Expensify.cash to iOS and Android native share menus #1361

Conversation

r8 commented Jan 29, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Screenshots

iOS

Android

github-actions bot commented Jan 29, 2021 • edited Loading

TomatoToaster commented Feb 2, 2021

r8 commented Feb 2, 2021

TomatoToaster commented Feb 2, 2021

r8 commented Feb 2, 2021

TomatoToaster 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

TomatoToaster left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

r8 commented Jan 29, 2021 •

edited

Loading

github-actions bot commented Jan 29, 2021 •

edited

Loading