[Android] Add support for vibration patterns #6400
Conversation
|
By analyzing the blame information on this pull request, we identified @skv-headless to be a potential reviewer. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
according to @satya164 note #6061 (comment) |
| if (Platform.OS === 'android') { | ||
| RCTVibration.vibrate(duration); | ||
| if (typeof pattern === "number") { |
There was a problem hiding this comment.
quotes: Strings must use singlequote.
skv-headless
force-pushed
the
vibration-patterns
branch
from
da2238b
to
47bedf1
Compare
|
@skv-headless updated the pull request. |
|
@satya164 could you please take a look? |
|
@skv-headless Hey, sorry, totally missed this. Will review tomorrow. Thanks for pinging me. |
| @@ -27,11 +27,31 @@ var Platform = require('Platform'); | |||
| */ | |||
|
|
|||
| var Vibration = { | |||
| vibrate: function(duration: number = 400) { | |||
| vibrate: function(pattern: number | Array<number> = 400, repeat: number) { | |||
There was a problem hiding this comment.
Can we mark repeat as optional? And maybe set the default value to 1?
|
Can the cancel method be supported on iOS? What'll happen if I do |
|
cc @dmmiller |
|
https://github.com/facebook/react-native/blob/master/Libraries/Vibration/RCTVibration.m#L20 |
skv-headless
force-pushed
the
vibration-patterns
branch
from
47bedf1
to
6059488
Compare
|
@skv-headless updated the pull request. |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
| @@ -27,11 +27,31 @@ var Platform = require('Platform'); | |||
| */ | |||
|
|
|||
| var Vibration = { | |||
| vibrate: function(duration: number = 400) { | |||
| vibrate: function(pattern: number | Array<number> = 400, repeat: number = -1) { | |||
There was a problem hiding this comment.
Why is the value of repeat -1? What happens when you set it to 0? Looks kinda weird to repeat something for -1 times.
There was a problem hiding this comment.
This is just a straight copy of Android (http://developer.android.com/reference/android/os/Vibrator.html#vibrate(long[], int))
That said, I tend to agree. I wonder if we should do just a boolean for repeat or not, and not allow for the direct indexing into the pattern for repeat.
There was a problem hiding this comment.
@dmmiller I think a number >= 0 is a reasonable API. 0 = don't repeat, n = repeat n times, of course we've to normalize the value to what Android understands.
In fact, the web vibration API doesn't have repeat option at all. I guess because it can be done using a loop and timeout, though not very convenient.
There was a problem hiding this comment.
@dmmiller Oh wait, it's the index of the pattern, not repeat n times. I think we should just change it to true | false and pass 0 | -1 to Android accordingly.
There was a problem hiding this comment.
http://developer.android.com/intl/ru/reference/android/os/Vibrator.html#vibrate(long[], int)
repeat the index into pattern at which to repeat, or -1 if you don't want to repeat.
true | false is easier api but we will lose ability to repeat since exact index.
There was a problem hiding this comment.
@skv-headless I think it should be okay. Let's keep the Java code as it is, and change the JS to pass -1 or 0. So that if we need it, we can change it in future, and think of a better API.
There was a problem hiding this comment.
sorry. Not sure how to do it
There was a problem hiding this comment.
RCTVibration.vibrateByPattern(pattern, repeat ? 0 : -1);
skv-headless
force-pushed
the
vibration-patterns
branch
from
6059488
to
dcf9cb8
Compare
|
@skv-headless updated the pull request. |
skv-headless
force-pushed
the
vibration-patterns
branch
from
dcf9cb8
to
05fa913
Compare
|
@skv-headless updated the pull request. |
skv-headless
force-pushed
the
vibration-patterns
branch
from
05fa913
to
1f9a309
Compare
|
@satya164 done |
satya164
changed the title
|
@facebook-github-bot shipit |
facebook-github-bot
added
GH Review: accepted
and removed
GH Review: review-needed
labels
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
ghost
closed this
in
Summary:and "not supported" warnings for ios Closes facebook#6400 Differential Revision: D3113988 fb-gh-sync-id: 169623f157ec59c38b4368cbc668c85201b67ed8 fbshipit-source-id: 169623f157ec59c38b4368cbc668c85201b67ed8
and "not supported" warnings for ios