Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple attachments on Android #161

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

geraintwhite
Copy link
Contributor

@geraintwhite geraintwhite commented Sep 1, 2020

Fixes #153.

I've tested this in my app using my patch via patch-package but haven't tested it properly standalone.

It uses FileProvider so might need to include some of #127.

Please can someone else test it too to make sure there's nothing else missing in this implementation?

@chirag04
Copy link
Owner

chirag04 commented Sep 2, 2020

@Relax594

@Relax594
Copy link
Contributor

Relax594 commented Sep 3, 2020

Instant crash on first run
Couldn't find meta-data for provider with authority de...provider

what am i missing?

EDIT: OK got it running. Please add the changes made in
android/src/main/AndroidManifest.xml
and
android/src/main/res/xml/provider_paths.xml
in #127 to this PR.

@geraintwhite
Copy link
Contributor Author

I've implemented the FileProvider the way https://github.com/react-native-community/react-native-share does it so it will work if the app has its own FileProvider implementation (otherwise the manifest merger will fail on build).

@Relax594
Copy link
Contributor

Relax594 commented Sep 3, 2020

Even better, good catch.

The only thing im not quite sure about is
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />

Shouldn't this be up to the user instead of having it inside the lib?

@geraintwhite
Copy link
Contributor Author

True, I only added it to mimic RNShare's implementation - I assume it would work without it?

@Relax594
Copy link
Contributor

Relax594 commented Sep 3, 2020

It does. If the user needs it however, its up to him to add the permission into the app's manifest. If you remove the permission from your commit, i think we're good to go.

@geraintwhite
Copy link
Contributor Author

Should we use androidx.core.content.FileProvider instead of android.support.v4.content.FileProvider?

@Relax594
Copy link
Contributor

Relax594 commented Sep 3, 2020

Does not make a big difference right now but looking into the future it might be a good decision to do that, yes.

@geraintwhite
Copy link
Contributor Author

@Relax594 all done!

@Relax594
Copy link
Contributor

Relax594 commented Sep 3, 2020

Thank you!

ping @chirag04 LGTM.

@chirag04 chirag04 merged commit 940b80d into chirag04:master Sep 3, 2020
@chirag04
Copy link
Owner

chirag04 commented Sep 3, 2020

awesome! appreciate your contributions 🙌

@geraintwhite geraintwhite deleted the patch-1 branch September 3, 2020 13:21
mergify bot pushed a commit to celo-org/celo-monorepo that referenced this pull request Nov 13, 2020
### Description

We weren't able to attach logs on certain devices. I suspect [this PR](chirag04/react-native-mail#161) fixed the issue, though I need someone else to confirm since it works fine on my device

fixes #5730
i1skn pushed a commit to celo-org/celo-monorepo that referenced this pull request Nov 13, 2020
### Description

We weren't able to attach logs on certain devices. I suspect [this PR](chirag04/react-native-mail#161) fixed the issue, though I need someone else to confirm since it works fine on my device

fixes #5730
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After Recent Multiple Attachment Update No Files Attach
3 participants