Skip to content

[Android] vibration module#6061

Closed
skv-headless wants to merge 2 commits into
react:masterfrom
skv-headless:vibration-android
Closed

[Android] vibration module#6061
skv-headless wants to merge 2 commits into
react:masterfrom
skv-headless:vibration-android

Conversation

@skv-headless

Copy link
Copy Markdown
Contributor

I will fix other notes from #2794 if I get positive feedback.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@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 Feb 21, 2016
Comment thread Libraries/Vibration/Vibration.js Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

parameter duration Missing annotation

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

parameter duration Missing annotation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use Flow to declare it's a number.

@nicklockwood

Copy link
Copy Markdown
Contributor

Cc: @dmmiller

Comment thread Libraries/Vibration/Vibration.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: e.g.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sorry didn't understand you 😞

@mkonicek

Copy link
Copy Markdown
Contributor

Looks good overall, thanks!

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

Comment thread Libraries/Vibration/Vibration.js Outdated

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-unused-vars: "invariant" is defined but never used

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

Comment thread Libraries/Vibration/Vibration.js Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is not that easy to make vibration duration in ios. I would rather make separate pr for that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nicklockwood http://stackoverflow.com/a/13047464/1410905 if it is good solution I can make ios vibration duration here.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You don't need to store this separately. It is available on ReactContextBaseJavaModule via getReactApplicationContext()

@dmmiller

Copy link
Copy Markdown

Can we also add comments about deprecating VibrationIOS since we will now just have one Vibration API?

@satya164

Copy link
Copy Markdown
Contributor

Since we're adding this, it would be great to support a vibration pattern. Seems pretty simple to implement - http://developer.android.com/reference/android/os/Vibrator.html

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

@skv-headless

Copy link
Copy Markdown
Contributor Author

@dmmiller I've fixed your notes.

@satya164

Copy link
Copy Markdown
Contributor

@skv-headless Feel free to do it in a follow-up PR :D

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

@skv-headless

Copy link
Copy Markdown
Contributor Author

I think this PR is finished. I've already started to work on vibration patters but I'm going to create separate PR for that.

@dmmiller

Copy link
Copy Markdown

@facebook-github-bot shipit

@dmmiller

Copy link
Copy Markdown

Cool, thanks. I've asked for the bot to accept it :)

@bestander

Copy link
Copy Markdown
Contributor

@facebook-github-bot shipit

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@mkonicek

mkonicek commented Mar 1, 2016

Copy link
Copy Markdown
Contributor

Failed to land because of failing tests. Luckily this should be easy, you just need to update BUCK files so that the code builds internally at fb where we use Buck.

See https://circleci.com/gh/facebook/react-native/3133

My guess is you just need to add a dependency here: https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/shell/BUCK

@skv-headless

Copy link
Copy Markdown
Contributor Author

@mkonicek is there any instruction how to launch tests locally? I'll update tomorrow.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@skv-headless updated the pull request.

@skv-headless

Copy link
Copy Markdown
Contributor Author

@mkonicek circle tests passed

@dmmiller

dmmiller commented Mar 2, 2016

Copy link
Copy Markdown

@facebook-github-bot shipit

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@skv-headless

Copy link
Copy Markdown
Contributor Author

@dmmiller if there are problems with shipping (maybe like this #6208 (comment)) I can create PR again

@dmmiller

dmmiller commented Mar 3, 2016

Copy link
Copy Markdown

I'll get it to land. Let me commandeer the diff locally and get it in.

@dmmiller

dmmiller commented Mar 3, 2016

Copy link
Copy Markdown

@facebook-github-bot shipit

@ghost ghost closed this in 1bab7c5 Mar 3, 2016
ghost pushed a commit that referenced this pull request Aug 26, 2016
Summary:
This is a revised follow up version from #8574 ( originally implemented in `objc` )
This PR change the implementation in JS suggested by javache

**motivation**

To supports vibration pattern like android.

The [iOS vibration implementation link](http://stackoverflow.com/questions/12966467/are-there-apis-for-custom-vibrations-in-ios/13047464#13047464) mentioned by skv-headless at #6061 (comment), which will not be accepted by apple since that implementation uses a private API. Thus, I use pure public API `NSTimer` to implement it.

**Note**

Since vibration time on iOS is not configurable, there are slightly differences with android.
for example:

**Android Usage:**
`Vibration.vibrate([0, 500, 200, 500])`
==> V(0.5s) --wait(0.2s)--> V(0.5s)

`Vibration.vibrate([300, 500, 200, 500])`
==> --wait(0.3s)--> V(0.5s) --wait(0.2s)--> V(0.5s)

**iOS Usage:**
if first argument is 0, it will not be included in pattern array.
( vibration
Closes #9233

Differential Revision: D3775085

Pulled By: javache

fbshipit-source-id: 370495857d5581399de32d2bed1ea1bcce193e9d
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Sep 14, 2016
Summary:
This is a revised follow up version from #8574 ( originally implemented in `objc` )
This PR change the implementation in JS suggested by javache

**motivation**

To supports vibration pattern like android.

The [iOS vibration implementation link](http://stackoverflow.com/questions/12966467/are-there-apis-for-custom-vibrations-in-ios/13047464#13047464) mentioned by skv-headless at react/react-native#6061 (comment), which will not be accepted by apple since that implementation uses a private API. Thus, I use pure public API `NSTimer` to implement it.

**Note**

Since vibration time on iOS is not configurable, there are slightly differences with android.
for example:

**Android Usage:**
`Vibration.vibrate([0, 500, 200, 500])`
==> V(0.5s) --wait(0.2s)--> V(0.5s)

`Vibration.vibrate([300, 500, 200, 500])`
==> --wait(0.3s)--> V(0.5s) --wait(0.2s)--> V(0.5s)

**iOS Usage:**
if first argument is 0, it will not be included in pattern array.
( vibration
Closes react/react-native#9233

Differential Revision: D3775085

Pulled By: javache

fbshipit-source-id: 370495857d5581399de32d2bed1ea1bcce193e9d
This pull request was closed.
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.

8 participants