IntentAndroid.openChooserWithOptions implements by deminoth · Pull Request #5476 · react/react-native

deminoth · 2016-01-22T05:31:59Z

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 · 2016-01-22T05:32:50Z

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

eslint-bot · 2016-01-22T05:42:00Z

no-trailing-spaces: Trailing spaces not allowed.

facebook-github-bot · 2016-01-22T07:00:16Z

@deminoth updated the pull request.

satya164 · 2016-01-22T21:17:24Z

Hey. Thanks a lot for the PR.

I've couple of questions and some feedback,

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 ?

About the argument order, generally options is the last argument. So I'd make title the first argument and options as the second.
What are the different types that can be passed, except text/plain? text/html?
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 · 2016-01-22T21:46:11Z

@satya164

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).

Completely agree
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 · 2016-01-22T21:55:14Z

@nicklockwood

Yup
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 · 2016-01-22T22:00:17Z

@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 · 2016-01-22T22:04:22Z

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

janicduplessis · 2016-01-22T22:39:47Z

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 · 2016-01-22T22:52:57Z

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 · 2016-01-22T23:03:21Z

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 · 2016-01-22T23:09:48Z

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 · 2016-01-23T00:00:35Z

@deminoth updated the pull request.

deminoth · 2016-01-23T01:07:04Z

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 · 2016-01-23T01:51:23Z

@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 · 2016-01-25T18:27:40Z

@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 · 2016-02-07T18:33:49Z

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 · 2016-02-08T15:36:58Z

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 · 2016-02-10T21:23:09Z

@deminoth Interested in working on a ShareModule?

deminoth · 2016-02-10T23:15:05Z

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

satya164 · 2016-02-11T07:14:03Z

@deminoth Yeah, will be great. Thanks.

deminoth · 2016-02-13T00:59:27Z

A new PR for ShareModule #5904

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

mkonicek · 2016-03-20T17:25:56Z

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 :)

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

Summary: revision of react/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. Closesreact/react-native#5904 Differential Revision: D3612889 fbshipit-source-id: 0e7aaf34b076a99089cc76bd649e6da067d9a760

mikach · 2016-08-03T13:36:15Z

do plan to implement image sharing on Android?

deminoth · 2016-08-04T07:24:40Z

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

IntentAndroid.openChooserWithOptions implements
6d87f97

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

eslint-bot reviewed Jan 22, 2016
View reviewed changes

Comment threadLibraries/Components/Intent/IntentAndroid.android.js Outdated
eslint-botJan 22, 2016
Copy link
Copy Markdown
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.

remove trailing spaces
d723428

satya164 added GH Review: needs-revision and removed GH Review: review-needed labels Jan 22, 2016

deminoth mentioned this pull request Feb 13, 2016
Add Share module #5904
Closed

mkonicek added the Triage - In Progress label Mar 20, 2016

mkonicek closed this Mar 20, 2016

deminoth deleted the intent-chooser branch July 25, 2016 09:29

Uh oh!

Conversation

deminoth commented Jan 22, 2016

Uh oh!

facebook-github-bot commented Jan 22, 2016

Uh oh!

eslint-botJan 22, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 22, 2016

Uh oh!

satya164 commented Jan 22, 2016

Uh oh!

nicklockwood commented Jan 22, 2016

Uh oh!

satya164 commented Jan 22, 2016

Uh oh!

nicklockwood commented Jan 22, 2016

Uh oh!

nicklockwood commented Jan 22, 2016

Uh oh!

janicduplessis commented Jan 22, 2016

Uh oh!

satya164 commented Jan 22, 2016

Uh oh!

nicklockwood commented Jan 22, 2016

Uh oh!

janicduplessis commented Jan 22, 2016

Uh oh!

facebook-github-bot commented Jan 23, 2016

Uh oh!

deminoth commented Jan 23, 2016

Uh oh!

janicduplessis commented Jan 23, 2016

Uh oh!

satya164 commented Jan 25, 2016

Uh oh!

haydenth commented Feb 7, 2016

Uh oh!

mkonicek commented Feb 8, 2016

Uh oh!

satya164 commented Feb 10, 2016

Uh oh!

deminoth commented Feb 10, 2016

Uh oh!

satya164 commented Feb 11, 2016

Uh oh!

deminoth commented Feb 13, 2016

Uh oh!

mkonicek commented Mar 20, 2016

Uh oh!

mikach commented Aug 3, 2016

Uh oh!

deminoth commented Aug 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants