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

IntentAndroid.openChooserWithOptions implements #5476

Closed
wants to merge 2 commits into from

Conversation

deminoth
Copy link
Contributor

This implementation of IntentAndroid.openChooserWithOptions is similar to ActionSheetIOS. showShareActionSheetWithOptions. It calls Intent.createChooser() and you can see how it acts at android docs

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek and @satya164 to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 22, 2016
* Open a chooser dialog to send data to other apps.
*
* Refer http://developer.android.com/intl/ko/training/sharing/send.html
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-trailing-spaces: Trailing spaces not allowed.

@facebook-github-bot
Copy link
Contributor

@deminoth updated the pull request.

@satya164
Copy link
Contributor

Hey. Thanks a lot for the PR.

I've couple of questions and some feedback,

  1. The name openChooserWithOptions is a bit confusing IMO. By chooser, I mostly understand that it'll pick a file or something as incoming data. But what this is doing is actually sending data out.

I'll prefer something like, showShareAction or similar. I dunno why the iOS one has WithOptions at the end though. Do you have any idea @ide @nicklockwood ?

  1. About the argument order, generally options is the last argument. So I'd make title the first argument and options as the second.

  2. What are the different types that can be passed, except text/plain? text/html?

  3. Should we have a universal ShareAction module which shows a share menu on both Android and iOS (it can just use ActionSheetIOS behind the scenes). cc @dmmiller

@nicklockwood
Copy link
Contributor

@satya164

  1. We should try to use similar terminology on both platforms. If there isn't already a well-understood term for this on Android the 'shareSheet' or 'actionsSheet' sounds better to my ear than 'chooser'.

The "withOptions" suffix is an Objective-C idiom, and I agree it's not appropriate for the JS api (we should remove it from ActionsSheetIOS).

  1. Completely agree

  2. We should definitey try to provide a common module for this sort of functionality, but I'd also like to try and merge the actionSheet and alertView apis on iOS (they use the same underlying native class), and I have a feeling that we should be trying to componentize these things, because these imperative alert/action apis, while faithful to the underlying platform apis, are not really in keeping with React design philosophy.

@satya164
Copy link
Contributor

@nicklockwood

  1. Yup

  2. One thing about componentizing. How do you think we should handle components which are usually singletons? For example, there can only be one <ActionSheet /> at a time, or only one <StatusBar />, or <Modal /> etc. Should we show a warning/error when multiple components are used even if only one is possible?

@nicklockwood
Copy link
Contributor

@satya164 they're not exactly singletons, because they can be stacked, but yes I think including more than one as a sibling should probably be an error.

@nicklockwood
Copy link
Contributor

@janicduplessis has a bit of a knack for componentizing these things, perhaps he'd like to have a go at it :-)

@janicduplessis
Copy link
Contributor

For something like ActionSheet it should be possible to have multiple ones but maybe only allow one to be visible at the time. Then having multiple ones open at the same time can be either an error, replace the existing one or have them open one after the other.

Another thing that can be used to componentize singleton APIs is something like I did with StatusBar and allow multiple components and merge their props then update the singleton with the merged values.

@satya164
Copy link
Contributor

I actually prefer the stacking idea better. We can have multiple components and only have the component at the top of the stack visible.

I am not in favor of merging props of different components, because then I can't just look at my component and predict what the state will be, which is a big advantage of React in the first place.

@nicklockwood
Copy link
Contributor

Merging props makes sense for the statusbar, but not for alerts. For example, you wouldn't want the topmost alert to show the message from the one underneath if you only specified a title!

@janicduplessis
Copy link
Contributor

More on topic of this PR, ActionSheetIOS.showShareActionSheetWithOptions and IntentAndroid.openChooserWithOptions could eventually converge into some sort of Share module (maybe with some other IntentAndroid features) that would stay imperative. ActionSheetIOS.showActionSheetWithOptions, and the Alert module could be merged and made as components.

That would require a lot of shuffling around the API :(

@facebook-github-bot
Copy link
Contributor

@deminoth updated the pull request.

@deminoth
Copy link
Contributor Author

Thank you for all the discussions.

Maybe I can make an universal Share module like @janicduplessis said. But I think IntentAndroid's other features should be merged with LinkingIOS.

@janicduplessis
Copy link
Contributor

@deminoth I know @satya164 was working on a cross platform LinkingIOS/IntentAndroid API, maybe the could be part of it with the iOS equivalent.

@satya164
Copy link
Contributor

@deminoth Yeah, I'm working on combining LinkingIOS and IntentAndroid to a single module named Linking #5336

I'd much prefer this to be a separate module rather than be part of the Linking module, because Linking deals with links :)

Something like ShareModule is good enough.

@haydenth
Copy link
Contributor

haydenth commented Feb 7, 2016

I desperately needed this in a project I am working on, so for the time being I implemented some of your code into a react module that will popup the default android share tray. Will be happy to deprecate when you build ShareModule or something like that.

https://github.com/haydenth/react-native-android-share

@mkonicek
Copy link
Contributor

mkonicek commented Feb 8, 2016

Thanks for releasing it @haydenth!

Had a quick look:

  • You could point people to rnpm in the installation instructions - it does all the steps needed to install a native module
  • Don't store a reference to the activity. The module is long-lived. Use the getCurrentActivity method instead. See for example IntentModule.java.

Looks like you could use some parts of this PR too.

@satya164
Copy link
Contributor

@deminoth Interested in working on a ShareModule?

@deminoth
Copy link
Contributor Author

@satya164 I was busy and just started working now. Should I provide an UIExplorer example too?

@satya164
Copy link
Contributor

@deminoth Yeah, will be great. Thanks.

@deminoth deminoth mentioned this pull request Feb 13, 2016
@deminoth
Copy link
Contributor Author

A new PR for ShareModule #5904

I want to use this PR but got stuck on Git merging...

@mkonicek
Copy link
Contributor

Hey @deminoth! Thanks for making the pull request, but we are closing it due to inactivity (37 days with no activity) to make sure all pull requests are either being worked on or closed. If you want to get your proposed changes merged, please rebase your branch with master and send a new pull request :)

@mkonicek mkonicek closed this Mar 20, 2016
@deminoth deminoth deleted the intent-chooser branch July 25, 2016 09:29
ghost pushed a commit that referenced this pull request Jul 25, 2016
Summary:
revision of #5476

It has only one method `shareTextContent` and next will be`shareBinaryContent`.

In Android, Promise can't receive a result, because `startActivityForResult` is not working with `Intent.ACTION_SEND`. Maybe we can use `createChooser(Intent target, CharSequence title, IntentSender sender)` which requires API level 22.
Closes #5904

Differential Revision: D3612889

fbshipit-source-id: 0e7aaf34b076a99089cc76bd649e6da067d9a760
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Jul 26, 2016
Summary:
revision of facebook/react-native#5476

It has only one method `shareTextContent` and next will be`shareBinaryContent`.

In Android, Promise can't receive a result, because `startActivityForResult` is not working with `Intent.ACTION_SEND`. Maybe we can use `createChooser(Intent target, CharSequence title, IntentSender sender)` which requires API level 22.
Closes facebook/react-native#5904

Differential Revision: D3612889

fbshipit-source-id: 0e7aaf34b076a99089cc76bd649e6da067d9a760
@mikach
Copy link

mikach commented Aug 3, 2016

do plan to implement image sharing on Android?

@deminoth
Copy link
Contributor Author

deminoth commented Aug 4, 2016

@mikach I don't have enough time for that now. I can help you if you make PR.

mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
revision of facebook#5476

It has only one method `shareTextContent` and next will be`shareBinaryContent`.

In Android, Promise can't receive a result, because `startActivityForResult` is not working with `Intent.ACTION_SEND`. Maybe we can use `createChooser(Intent target, CharSequence title, IntentSender sender)` which requires API level 22.
Closes facebook#5904

Differential Revision: D3612889

fbshipit-source-id: 0e7aaf34b076a99089cc76bd649e6da067d9a760
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants