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

Add vibration support for android #2794

Closed
wants to merge 2 commits into from
Closed

Add vibration support for android #2794

wants to merge 2 commits into from

Conversation

christopherdro
Copy link
Contributor

Add Vibration with duration for Android.
Tested basic implementation in a sample app before porting over to it's own module.

I'm not 100% sure if I got everything correct since some of the changes are part of the shell, but looking at how some of the other native modules were implemented it should be correct.

@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 Sep 17, 2015
@PhilippKrone
Copy link
Contributor

Currently developing some of my native modules for android as well and have a basic question concerning that, as I see exactly the same topic over here as well:

Shouldn't we rename "VibrationIOS" to "Vibration" only, thus having a "Vibration.ios.js" and a "Vibration.android.js". We do have several "IOS" APIs, so it's more of a general decision, isnt it?

Ideally, you can use "Vibration.vibrate()" both, for IOS and for Android, where as the is just ignored in the iOS version.

Regards
Philipp

@christopherdro
Copy link
Contributor Author

I decided to go with the flow of how the other components are set up.

We can see if the team decides to change the naming convention for the native modules but I don't think it's bad to separate them out, especially since each might have their own set of properties or parameters based on the systems requirements.

'use strict';

var warning = require('warning');
var RCTVibrationAndroid = require('NativeModules').VibrationAndroid;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use the RCT prefix in JS, it's an Obj-C convention we're still using in JS and we should move away from using it in JS.

@mkonicek
Copy link
Contributor

For APIs that are (almost) identical on iOS and Android, we prefer having only one implementation, with platform-specific annotations, see e.g. https://facebook.github.io/react-native/docs/text.html#content

In case the APIs diverge a lot, there should be FooAndroid, FooIOS but don't think this is the case for Vibration.

@simonexmachina
Copy link

What's the status of this feature? It looks to me that I still need to use a module for vibration on Android. Surely this can be handled using a single interface that works for both platforms?

ghost pushed a commit that referenced this pull request Mar 3, 2016
Summary:I will fix other notes from #2794 if I get positive feedback.
Closes #6061

Reviewed By: nicklockwood

Differential Revision: D2982173

Pulled By: dmmiller

fb-gh-sync-id: d1e9407798b0293b090897a10996085b0f0c1b3e
shipit-source-id: d1e9407798b0293b090897a10996085b0f0c1b3e
pglotov pushed a commit to pglotov/react-native that referenced this pull request Mar 15, 2016
Summary:I will fix other notes from facebook#2794 if I get positive feedback.
Closes facebook#6061

Reviewed By: nicklockwood

Differential Revision: D2982173

Pulled By: dmmiller

fb-gh-sync-id: d1e9407798b0293b090897a10996085b0f0c1b3e
shipit-source-id: d1e9407798b0293b090897a10996085b0f0c1b3e
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.

6 participants