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

r8 · 2021-02-05T00:38:37Z

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-02-05T00:38:53Z

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

r8 · 2021-02-08T21:39:00Z

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.

Hey @marcaaron
I will move the discussion here, because I'm continuing work on it in this new pull request.
And you have replied to already merged one, which disabled comments for me and I'm not able to reply.

src/libs/actions/SharedItem.js

r8 · 2021-02-08T22:49:33Z

src/pages/SharePage.js

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


We probably don't need the CreateMenu here?
Same with the FAB?

@marcaaron
CreateMenu and FAB would allow user to share not only to the chat from the list, but also to create a new chat and share there.
Of course a new chat can be created also with a search functionality of ChatSwitcherView, but that's probably less intuitive for the user.

OK, I see that new Search view has been merged to the master, so most of this doesn't make sense anymore and should be reworked anyway :)

marcaaron · 2021-02-12T20:30:16Z

@r8 sorry for missing a few of these replies please let us know if you are still working on this, thanks!

r8 · 2021-02-12T20:38:27Z

Hey @marcaaron
Sorry, it takes so long. I was a bit overloaded for a last few days, so wasn't able to make any significant progress
I'm still willing to finish it and will work on it during this weekend, if it's ok with you.

marcaaron · 2021-02-12T20:41:45Z

Sounds good! Thanks for the update.

…earch Page

r8 · 2021-02-15T20:54:10Z

@marcaaron Could you please take a look? What do you think about this approach?
Currently I'm just showing recent chats with OptionsList.

I tried to reuse new Search components, then I tried to extend Home Page components to implement the sharing directly there, but had to revert both times and go the simper way.
Unfortunately everything there is too tightly coupled with chat functionality, so it's not possible to reuse chat selection/creation components without major refactoring.

marcaaron · 2021-02-15T22:06:03Z

I tried to reuse new Search components, then I tried to extend Home Page components to implement the sharing directly there, but had to revert both times and go the simper way.
Unfortunately everything there is too tightly coupled with chat functionality, so it's not possible to reuse chat selection/creation components without major refactoring.

Can you explain what sorts of problems you ran into here? I'm not sure if I completely understand what kind of refactoring you are suggesting we need. But I think the approach you have here that relies on OptionsList makes sense to me, but I would expect to be able to search for a particular use to share something with so I would model this off of the Share view.

r8 · 2021-02-16T01:33:06Z

I'm not actually suggesting, I'm more like thinking out loud.
As you know, usually React apps have two kinds of components: Smart and dumb (or Container and Presentational).
Smart ones are connected to the state and actions. Dumb ones doesn't know much about global state and actions, and just using whatever parent smart component passes down.

The biggest challenge of this task for me is that almost each component I tried to reuse was a Smart one. I couldn't just reuse SidebarLinks, for example, because it's talking to the Onyx and calling redirect actions directly.

Anyway, I should probably use OptionsSelector instead of OptionsList, so user would be able to search for the contact. And maybe even show sections with Recent chats and Contacts. More like a hybrid of SearchPage and NewGroupPage

marcaaron · 2021-02-16T02:58:27Z

I'm not actually suggesting, I'm more like thinking out loud.

Ah, my apologies. Yes, I think using OptionsSelector should work for this. Anyways, if you do have some ideas about how to refactor some things we'd love to hear about it. We are all pretty comfortable with duplication if it saves us from premature optimization, but maybe there are some views that can be generalized some more into some dumb components.

r8 · 2021-02-24T22:35:40Z

I've updated the code according to comments.
But on a global level, should I rework the UI so it would be closer to Slack/WhatsUp?

nickmurray47 · 2021-02-25T19:18:53Z

I've updated the code according to comments.
But on a global level, should I rework the UI so it would be closer to Slack/WhatsUp?

I think the Slack/WhatsApp design would be ideal @r8, thanks for the latest changes. Let's get our design team in here to confirm, too. cc @Expensify/design

marcaaron · 2021-02-25T20:49:16Z

But on a global level, should I rework the UI so it would be closer to Slack/WhatsUp?

Sorry, can you clarify what you mean by this exactly?

r8 · 2021-02-25T21:09:17Z

@marcaaron
I'm referring to @chiragsalian comment

But the steps mentioned in the issue are not really reflective of whatsapp or slack. Wherein in those two the mobile share modal first gathers the information like to who we are sharing with and the title of the image before opening the app.
But in this code we are asking for what seems to be the title first and then opening the app to select the user to share with. Kinda different but I'm not sure how much we care about this. Any thoughts on this @marcaaron? Just making sure we are getting it right the first time.

marcaaron · 2021-02-25T21:21:55Z

@r8 Sorry I don't have the answer to that. Maybe @shawnborton or @sakluger can weigh in on how this should work?

marcaaron

I'm gonna leave this review for @chiragsalian and @nickmurray47 to complete.

One comment I will reiterate here is that we do not have any async actions so I think if we can avoid making getOrCreateChatReport() return a Promise that would be ideal. It breaks some things about the philosophy of how Onyx should work.

See: https://github.com/Expensify/Expensify.cash/blob/master/README.md#philosophy

This case seems a benign, but I think it's a practice we are trying to completely avoid since once the flood gates are open it is much more likely that we will have lots of synchronous code. I also don't really have a clean solution other than try a version of this where we:

redirect() to the report screen
once there check for the sharedItem and perform the logic to save a draft or upload the attachment

marcaaron · 2021-02-25T20:56:15Z

src/libs/ShareManager/index.native.js

+ * @returns {Object} Prepared shared item structure
+ */
+function prepareSharedItem(sharedItem) {
+    if (sharedItem.mimeType.match(TEXT_MIME_TYPE)) {


+1 seems fine to me

r8 · 2021-02-25T21:43:10Z

@marcaaron

I also don't really have a clean solution other than try a version of this where we:

redirect() to the report screen

once there check for the sharedItem and perform the logic to save a draft or upload the attachment

I was actually considering this approach. But I didn't like it.
Currently all sharing functionality is encapsulated in one place, ShareScreen only (with dependency on ShareManager).
But if I would add this check to Report screen, it means that functionality will be smeared on several screens.
Which is actually another flood gate I wouldn't like to open :)

sakluger · 2021-02-25T22:17:05Z

But the steps mentioned in the issue are not really reflective of whatsapp or slack. Wherein in those two the mobile share modal first gathers the information like to who we are sharing with and the title of the image before opening the app.

That's not what I see in iOS. If I click Share from a page in Safari (my default browser) and then select WhatsApp or Slack, it pulls up a modal from the actual WhatsApp or Slack app. Here's what WhatsApp looks like:

And here's what Slack looks like:

So I think that the issue description is still a good way to implement this. When they select to share to Expensify.Cash, we open up the homepage of the ECash app, which lets them choose who to share with.

r8 · 2021-02-25T22:21:23Z

@marcaaron
But as I wrote before, I'm more used to industry standards. ReactNavigation, Redux etc.
You are using your own innovative tech (like Onyx) and your own architecture.
The main challenge of this task for me, is that I have to go outside of my comfort zone and think out of the box (unlike the previous tasks I did for the Expensify).
So I'm not the right person to make an architectural decisions. And if you prefer to add this logic you've mentioned to Report Screen and it will be ok with your architecture, I can do it.

marcaaron · 2021-02-25T22:30:37Z

it means that functionality will be smeared on several screens. Which is actually another flood gate I wouldn't like to open :)

Yep, I understand and agree. But perhaps, we could make it a pure function that lives inside the SharedItem actions file and then just call something like...

componentDidMount() {
    if (props.sharedItem) {
        processSharedItem(reportID, sharedItem);
    }
}

I'll admit, there are some things that make this a bit convoluted at the moment like the way we are loading several reports in the background here (so it would require another check in componentDidUpdate too to see if the report has become active):

https://github.com/Expensify/Expensify.cash/blob/38b392173a5e990a0d0e5fe9b327a14f7ae722ad/src/pages/home/ReportScreen.js#L94-L106

That particular problem will be solved by the addition of react-navigation which is in progress now (but that's a whole other issue).

But as I wrote before, I'm more used to industry standards. ReactNavigation, Redux etc.
You are using your own innovative tech (like Onyx) and your own architecture.
The main challenge of this task for me, is that I have to go outside of my comfort zone and think out of the box (unlike the previous tasks I did for the Expensify).
So I'm not the right person to make an architectural decisions. And if you prefer to add this logic you've mentioned to Report Screen and it will be ok with your architecture, I can do it.

That all makes sense to me. I know we have decided as a team to pursue something novel and learning as we go so we appreciate your patience. It's more important to us to try and stick to doing things one way for now. If that way ends up being wrong then we'll switch things up. Even this decision is not 100% mine to make in this case :)

sakluger · 2021-02-25T23:08:31Z

Just chatted with @chiragsalian separately and I wanted to add another comment with the desired flow. In the screenshots I posted above from the WhatsApp and Slack share modals, the user selects who to send it to (either an individual or a group). We'd like to see similar behavior.

So my thinking was that when you select to share something, it would pop up with a modal that looks something like this:

You could choose from one of the recent chats, or you could search or start a new chat or group. As soon as you select a chat, it would send your content to that chat. @marcaaron @chiragsalian does that sound like what you two were thinking of too?

marcaaron · 2021-02-26T01:11:53Z

You could choose from one of the recent chats, or you could search or start a new chat or group. As soon as you select a chat, it would send your content to that chat

Of course, this change can be done and would be easier to pull off once the change I suggested above (i.e. to make the act of navigating to a report screen with a share item upload to the report). But I also think we should do this in a follow up PR.

I'm going to advocate for @r8 here a bit and say that it's a little unreasonable to ask for this change at this point. If we wanted this change we should have asked for it 20+ days ago when this PR was created. This is the proposal we agreed to move forward with so we should merge this and create a new issue to allow for what you're asking.

marcaaron · 2021-02-26T01:14:03Z

Also, I will take some responsibility for that since looking at the original issue I'm not sure I understand clearly what you were asking for @sakluger 😅

sakluger · 2021-02-26T18:24:06Z

That's totally fair @marcaaron!

I reviewed the demo video again and the only concern I have is that the button on the first step says "Post", which makes me think that it would post the content immediately, before getting a chance to select the chat.

Could we change that "Post" to say "Next" instead? That would make it clear that there's another step (i.e. selecting the chat) before posting.

If we made this small change, then I'd feel totally comfortable merging.

r8 · 2021-03-01T22:05:36Z

I've done a bit of investigation, and here is what I know so far:

This 'Cancel' and 'Post' buttons are defined in iOS core library for the share functionality (SLComposeServiceViewController).
There's no official way to change the label, but there are some hacks.
For example, see https://stackoverflow.com/questions/27605412/share-extension-cancel-and-post-buttons

So I'm in the big dilemma.

The controller for the share functionality is provided by the package I'm using - react-native-share-menu. See https://github.com/meedan/react-native-share-menu/blob/master/ios/ShareViewController.swift
It means that the only way to redefine the label is to copy this file from the package, add the hacks and save it directly in the project. But it's a bad practice to hack the dependencies this way, and also would create problems with upgrading the dependency etc.

Second option is to build a custom share view. But that's a bit of a windy and scary road with modification of build phases for the extension and with export of the share component from Expensify.chat app.
See https://github.com/meedan/react-native-share-menu/blob/master/SHARE_EXTENSION_VIEW.md

And yet another option (to make it a trilemma) is to follow the second approach, but export the dummy component,
like here: Expensify/react-native-share-menu#82 (comment)
That will make the iOS share functionality to behave exactly like on android, launching Expensify.chat app without any Share modal.

Could you please advise what would be the preferable solution in this case?

marcaaron · 2021-03-03T02:35:09Z

There's no official way to change the label, but there are some hacks
it's a bad practice to hack the dependencies this way, and also would create problems with upgrading the dependency etc

I think we can rule out hacks as an option

Second option is to build a custom share view

This does look scary. But maybe not completely off the table.

And yet another option (to make it a trilemma) is to follow the second approach, but export the dummy component

This also seems reasonable.

Probably if we go this way we should still proceed with what we have and maybe follow up with this improvement in another issue. It looks fairly involved and I think there are some good building blocks laid out here. Thoughts @sakluger ?

I might also propose another option which would be to submit a fix to the main library so that a custom share view isn't necessary and iOS can optionally behave like Android. But not sure if this is what we want at the end of the day.

marcaaron · 2021-03-19T16:58:29Z

Not sure if there is any update here. This PR has got some pretty bad conflicts now. The good news is we have updated to react-navigation so maybe some of this plan would be easier to pull off. But the original concerns about the library are still unresolved.

r8 · 2021-03-19T18:11:43Z

@marcaaron oh, you're right. Sorry.
I'll fix the conflicts and check how this can be changed with react-navigation

…h into sergey-share-menu-fixes � Conflicts: � src/ONYXKEYS.js

marcaaron · 2021-04-09T19:33:04Z

Let us know how you want to proceed @r8. Thanks!

r8 · 2021-04-10T23:46:20Z

@marcaaron To be honest, now I have much less time and energy than I had when it all started.
This task feels like a moving target. It went through two pull requests, several architecture changes in master, numerous conflict resolutions, and two big rewrites. And yet I haven't been able to finish it. And now there's another big architecture change, so it has to be rewritten again.
Probably let's just agree that I've failed here, and go our separate ways.

marcaaron · 2021-04-12T15:37:05Z

This task feels like a moving target. It went through two pull requests, several architecture changes in master, numerous conflict resolutions, and two big rewrites. And yet I haven't been able to finish it. And now there's another big architecture change, so it has to be rewritten again.

Ah no worries. Yep I think maybe if we try to do this again we will move forward in smaller increments.

Probably let's just agree that I've failed here, and go our separate ways.

I think it's understandable to run out of steam or know when to cut losses on a PR. I wouldn't exactly call this a failure since we've learned some interesting things about this feature. Anyways, feel free to pick up smaller jobs if you have the time. We have enjoyed working with you.

r8 added 2 commits February 5, 2021 02:25

Add Expensify.cash to iOS and Android native share menus

b785ce9

Fix invalid SafeAreaProvider import

1da851a

r8 added 2 commits February 8, 2021 23:15

Replace condition with early return in handleShare

0a9caf3

Set share listener to undefined after removing

9e30961

r8 commented Feb 8, 2021

View reviewed changes

src/libs/actions/SharedItem.js Show resolved Hide resolved

marcaaron assigned r8 Feb 8, 2021

marcaaron requested review from a team and removed request for a team February 8, 2021 21:51

botify requested a review from nickmurray47 February 8, 2021 21:52

r8 commented Feb 8, 2021

View reviewed changes

r8 added 4 commits February 14, 2021 22:06

Merge branch 'master' into sergey-share-menu-fixes

ca27112

Revert changes that are no longer valid after introduction of a new S…

9cc0a09

…earch Page

Move SharedItem PropTypes to ShareManager

ba889d2

Reimplement the Share Page with OptionsList

4dc9544

r8 added 4 commits February 16, 2021 23:56

Use OptionsSelector with Search for Share Page

5b89dfe

Create a new report on Share Page if selected one doesn't exist

dd9e220

Merge branch 'master' into sergey-share-menu-fixes

29be8f3

Fix linter issue

8dfb623

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 Feb 16, 2021

r8 marked this pull request as ready for review February 16, 2021 22:55

r8 requested a review from a team as a code owner February 16, 2021 22:55

r8 added 4 commits February 23, 2021 01:04

Fix return type doc for fetch chat report function

5dc9081

Fix url scheme for iOS

11d95d2

Revert Cocoapods version change

b52a42d

Merge branch 'master' into sergey-share-menu-fixes

a127379

marcaaron reviewed Feb 25, 2021

View reviewed changes

r8 added 3 commits March 23, 2021 00:04

Merge branch 'master' into sergey-share-menu-fixes

f6bfc95

Return react-native-share-menu to package-lock.json

bbd1e09

Merge branch 'sergey-share-menu-fixes' of github.com:r8/Expensify.cas…

bee451d

…h into sergey-share-menu-fixes � Conflicts: � src/ONYXKEYS.js

r8 marked this pull request as draft March 22, 2021 22:43

marcaaron closed this Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

r8 commented Feb 5, 2021 •

edited

Loading

github-actions bot commented Feb 5, 2021 •

edited

Loading

r8 commented Feb 8, 2021

r8 Feb 8, 2021

r8 Feb 8, 2021

marcaaron commented Feb 12, 2021

r8 commented Feb 12, 2021

marcaaron commented Feb 12, 2021

r8 commented Feb 15, 2021

marcaaron commented Feb 15, 2021

r8 commented Feb 16, 2021

marcaaron commented Feb 16, 2021

r8 commented Feb 24, 2021

nickmurray47 commented Feb 25, 2021 •

edited

Loading

marcaaron commented Feb 25, 2021

r8 commented Feb 25, 2021

marcaaron commented Feb 25, 2021

marcaaron left a comment

marcaaron Feb 25, 2021

r8 commented Feb 25, 2021

sakluger commented Feb 25, 2021

r8 commented Feb 25, 2021

marcaaron commented Feb 25, 2021

sakluger commented Feb 25, 2021

marcaaron commented Feb 26, 2021

marcaaron commented Feb 26, 2021

sakluger commented Feb 26, 2021

r8 commented Mar 1, 2021 •

edited

Loading

marcaaron commented Mar 3, 2021

marcaaron commented Mar 19, 2021

r8 commented Mar 19, 2021

marcaaron commented Apr 9, 2021

r8 commented Apr 10, 2021

marcaaron commented Apr 12, 2021

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

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

Conversation

r8 commented Feb 5, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Screenshots

iOS

Android

github-actions bot commented Feb 5, 2021 • edited Loading

r8 commented Feb 8, 2021

r8 Feb 8, 2021

Choose a reason for hiding this comment

r8 Feb 8, 2021

Choose a reason for hiding this comment

marcaaron commented Feb 12, 2021

r8 commented Feb 12, 2021

marcaaron commented Feb 12, 2021

r8 commented Feb 15, 2021

marcaaron commented Feb 15, 2021

r8 commented Feb 16, 2021

marcaaron commented Feb 16, 2021

r8 commented Feb 24, 2021

nickmurray47 commented Feb 25, 2021 • edited Loading

marcaaron commented Feb 25, 2021

r8 commented Feb 25, 2021

marcaaron commented Feb 25, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Feb 25, 2021

Choose a reason for hiding this comment

r8 commented Feb 25, 2021

sakluger commented Feb 25, 2021

r8 commented Feb 25, 2021

marcaaron commented Feb 25, 2021

sakluger commented Feb 25, 2021

marcaaron commented Feb 26, 2021

marcaaron commented Feb 26, 2021

sakluger commented Feb 26, 2021

r8 commented Mar 1, 2021 • edited Loading

marcaaron commented Mar 3, 2021

marcaaron commented Mar 19, 2021

r8 commented Mar 19, 2021

marcaaron commented Apr 9, 2021

r8 commented Apr 10, 2021

marcaaron commented Apr 12, 2021

r8 commented Feb 5, 2021 •

edited

Loading

github-actions bot commented Feb 5, 2021 •

edited

Loading

nickmurray47 commented Feb 25, 2021 •

edited

Loading

r8 commented Mar 1, 2021 •

edited

Loading