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

feat: add optional haptic feedback #5

Merged
merged 4 commits into from
Mar 8, 2021
Merged

Conversation

ksitko
Copy link
Contributor

@ksitko ksitko commented Mar 8, 2021

Motivation

I was hoping to start a discussion about haptic feedback. This library nails down the "pop" for a hold menu wonderfully, but it's hard to not notice that it's missing any sort of haptics. I found that you at one point added expo-haptics in c3e8105 then removed it the next commit. Would you mind sharing as to why?

This somewhat opinionated PR is dependent on react-native-haptic-feedback and in order to experience the haptics you have to installed it. But it isn't necessary to have it installed for the react-native-hold-menu library to work. But it does give a quick and easy out of the box experience for users of this library. Let me know what you think and if this is a path you'd like to go down.

@enesozturk
Copy link
Owner

Hi @ksitko, thanks for another contribution for an important feature. As you said, I tried it with expo-haptic and also react-native-haptic-feedback. Because of other issues it planned to complete it in further releases.

I understand your concern about installment. We can add it to the docs to let user know they have to install it if needed. I will check your PR and try examples with haptic. Will let you know. Thanks! 🙌🏽

@gorhom
Copy link
Collaborator

gorhom commented Mar 8, 2021

@ksitko @enesozturk you could use the new metro feature optional dependency to pick react-native-haptic-feedback or expo-haptic.

facebook/metro#511

@enesozturk
Copy link
Owner

Hi @gorhom, thanks for let us know the metro's that feature. As I understood that feature is what @ksitko's used here right?

@gorhom
Copy link
Collaborator

gorhom commented Mar 8, 2021

@enesozturk ahh sorry did not check the code 😅

As I understood that feature is what @ksitko's used here right?

exactly

@enesozturk
Copy link
Owner

enesozturk commented Mar 8, 2021

Ah, ok than 😄

expo-haptic may be easier to use for Expo users, I will consider that anyway.. thanks 🙂👍🏽

@ksitko
Copy link
Contributor Author

ksitko commented Mar 8, 2021

I personally didn't like expo-haptic too much when I was looking for haptic libraries a long time ago but I should double check it again. Now that I think about it, given that expo 41 is going to include the reanimated v2 release it would probably be best to try to keep the library expo managed workflow compatible. Unless there are already some other dependencies which would prevent that.

Regardless of what library, what do you think about having haptics be an out of the box default such that so long as whatever haptics library has been installed, react-native-hold-menu provides users with a nice haptics response as default behavior?

@enesozturk
Copy link
Owner

@ksitko I checked the haptic feedback and it works pretty well. Looks like react-native-haptic-feedback will not work in Expo projects and we'll needed to provide support for expo-haptic as well. mkuczera/react-native-haptic-feedback#50

When I first try haptics, it was like how you are telling actually. Letting user call any function with a callback like onActivate and user will make haptic call. Does this do what you are saying?

const hapticFunction = ()=>{
   // Call expo-haptic or react-native-haptic-feedback methods
}

<HoldMenu onActivate={hapticFunction} />
// This is going to be called in onCompletion in HoldItem.tsx

@ksitko
Copy link
Contributor Author

ksitko commented Mar 8, 2021

When I first try haptics, it was like how you are telling actually. Letting user call any function with a callback like onActivate and user will make haptic call. Does this do what you are saying?

Not entirely. I'm suggesting is that instead of requiring the user to configure and provide a hapticFunction, react-native-hold-menu should have one by default. A prop could control or disable it, but have it out of the box, just like with the blur view.

@ksitko
Copy link
Contributor Author

ksitko commented Mar 8, 2021

Ah I just realized that this library already has a dependency of expo-blur, so since you already are requiring react-native-unimodules you may as well just include expo-haptics as well. 😅

@enesozturk
Copy link
Owner

Not entirely. I'm suggesting is that instead of requiring the user to configure and provide a hapticFunction, react-native-hold-menu should have one by default. A prop could control or disable it, but have it out of the box, just like with the blur view.

This time it will needed to be write native module and it effects the installation process as well as I understood.

Ah I just realized that this library already has a dependency of expo-blur, so since you already are requiring react-native-unimodules you may as well just include expo-haptics as well. 😅

Yes 😄 I think expo-haptics would be better if it supports bare RN solutions. Trying it now

@ksitko
Copy link
Contributor Author

ksitko commented Mar 8, 2021

I just replaced react-native-haptic-feedback with expo-haptics in the latest commit, give that a shot. It adds expo-haptics as a dependency for react-native-hold-menu just like expo-blur.

@enesozturk
Copy link
Owner

enesozturk commented Mar 8, 2021

You already committed... 😄 Checking

[Edit]: Perfect 👌🏽 🙂 @ksitko

@enesozturk enesozturk merged commit 46737ad into enesozturk:develop Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants