-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Conversation
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 |
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; |
There was a problem hiding this comment.
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.
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 |
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? |
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
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.