[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) |
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. |
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
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 react#6400 Differential Revision: D3113988 fb-gh-sync-id: 169623f157ec59c38b4368cbc668c85201b67ed8 fbshipit-source-id: 169623f157ec59c38b4368cbc668c85201b67ed8
5 participants
and "not supported" warnings for ios