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

[HOLD for payment 2021-11-09] Reduce the haptic feedback vibration to a short bip #4355

Closed
isagoico opened this issue Aug 1, 2021 · 57 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Aug 1, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Upwork posting: https://www.upwork.com/jobs/~0105c031baf3dfd3df


Action Performed:

  1. Navigate to a conversation in mobile
  2. Press and hold on a message

Expected Result:

There should be a short blip of vibration.

Actual Result:

Current vibration is longer that it should be.

To achieve the desired result, we should:

  1. Update our app to use react-native-haptic-feedback
  2. Fork the library, and update the forked lib to have the same vibration as slack does when selecting messages
  3. Once the PR is merged into our codebase, create a PR upstream so that the functionality can be merged into react-native-haptic-feedback

Workaround:

N/A

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.0.82-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

View all open jobs on Upwork


From @quinthar https://expensify.slack.com/archives/C01GTK53T8Q/p1627669368027700

I love the new haptic feedback when you long tap on mobile. However, it uses a kind of strange vibration. Can we match the same vibration that slack uses with long tap? It's just a incredibly short blip.

@MelvinBot
Copy link

Triggered auto assignment to @pecanoro (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@pecanoro pecanoro added Weekly KSv2 Improvement Item broken or needs improvement. and removed Daily KSv2 labels Aug 2, 2021
@pecanoro pecanoro removed their assignment Aug 2, 2021
@pecanoro pecanoro added the External Added to denote the issue can be worked on by a contributor label Aug 2, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Jag96 (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Aug 2, 2021
@Jag96 Jag96 added the Exported label Aug 2, 2021
@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 2, 2021
@Jag96
Copy link
Contributor

Jag96 commented Aug 2, 2021

It looks like there's some more context on this issue in this slack thread. cc @rushatgabhane since you had some good thoughts here.

@rushatgabhane
Copy link
Member

@Jag96 I'm not able to find any npm package which matches the short blip of Slack.

However, I can get the exact haptic feedback as Slack's using native code for Android.

Here are our options for adding native code.

  1. Directly into New Expensify using NativeModules.
  2. Create a new npm package.

Which one do you prefer?

Alternatively, we could settle for a shorter duration of the strange vibration using rn-haptic-feedback.

@Jag96
Copy link
Contributor

Jag96 commented Aug 3, 2021

We've used NativeModules before here so I think that would be fine, but I'd reply to the thread in slack (and send to the main channel as well by ticking Also send to expensify-open-source) just to see if anybody has any opinions.

And just to confirm, using NativeModules would you just override the index.android.js functionality and continue using the current lib for iOS?

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 3, 2021

And just to confirm, using NativeModules would you just override the index.android.js functionality and continue using the current lib for iOS?

No. I won't using the current lib (expo-haptics) at all.
NativeModules will be used for both iOS and Android.

@rushatgabhane

This comment has been minimized.

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 3, 2021

Update from slack thread

We decided to add bip() method to Vibration API by creating a PR for React Native.

I won't be available for a few weeks. So I won't be taking this issue.
But here are things that might help with implementation.

Android: Vibrate(createPredefined(VibrationEffect.EFFECT_CLICK))
iOS: Selection

Correction: bip() not blip()

@quinthar
Copy link
Contributor

quinthar commented Aug 3, 2021 via email

@MelvinBot
Copy link

@Jag96 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@arpitdeveloper
Copy link

arpitdeveloper commented Aug 7, 2021

In this case we add the code

{<TouchableOpacity
         
         onLongPress={() => Vibration.vibrate(1 * ONE_SECOND_IN_MS)}
     >
....
</TouchableOpacity>}

in InvertedFlatList in "/src/pages/home/report/ReportActionsView.js"

@chira37
Copy link

chira37 commented Aug 7, 2021

change src\components\PressableWithSecondaryInteraction\index.native.js


import {Vibration} from 'react-native';

<Pressable
...
        onLongPress={(e) => {
            e.preventDefault();
            Vibration.vibrate(20);
            props.onSecondaryInteraction(e);   
        }}
...
    >
        {props.children}
    </Pressable> ````

in src\components\PressableWithSecondaryInteraction

it will reduce time from 50 (with haptics library) millisecond to 20 millisecond

@Jag96
Copy link
Contributor

Jag96 commented Aug 9, 2021

@arpitdeveloper @chira37 please see the description of this issue for the desired implementation:

To achieve the desired result, we should:

  1. Update the Vibration lib in our react native fork to add a new function bip that will use the shortest, strongest vibration possible (see slack thread)
  2. Once the PR is merged, create a PR upstream so that the functionality can be merged into React Native's Vibration library

@MelvinBot MelvinBot removed the Overdue label Aug 9, 2021
@silverfoxgit
Copy link

We can reduce the time of vibration.
This is possible in React Native.

@Jag96
Copy link
Contributor

Jag96 commented Sep 21, 2021

Still held on n6, clearing overdue label

@Jag96
Copy link
Contributor

Jag96 commented Sep 30, 2021

Still held on n6

@MelvinBot MelvinBot removed the Overdue label Sep 30, 2021
@kadiealexander
Copy link
Contributor

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

@rushatgabhane
Copy link
Member

@Jag96 should I submit a PR to NewDot?

--
I've taken the liberty to add new methods to rn-haptic-feedback (like effectClick, effectHeavyClick etc).
And also made the code more modular. PR link

I hoped that my PR to rn-haptic-feedback would have been merged by now.
But there has been no reply on it for 4 weeks.

Is there anything that I can do to get it merged faster?
Should I email a maintainer?

@Jag96
Copy link
Contributor

Jag96 commented Oct 12, 2021

@rushatgabhane this is still on n6-hold, and we'll need to create an Expensify fork of rn-haptic-feedback first for this where you can then submit a PR, so I think we can hold for now. If the n6-hold label comes off and the PR still hasn't been reviewed in the rn-haptic-feedback repo then we can get that repo forked by Expensify, have you create a PR on the Expensify fork, get it reviewed/tested, and then make a PR in NewDot to use that fork in our package.json.

Going to assign you the issue here, re-create the Upwork posting since it looks like it expired, and hire you for the job.

Since there hasn't been any reply for a bit I think it's fine to email the maintainer to see if they missed it, you can also try requesting a review from mkuczera (and tagging them in the PR description) since it looks like they have reviewed many of the more recent PRs merged to the repo.

@MelvinBot MelvinBot removed the Overdue label Oct 12, 2021
@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 12, 2021
@rushatgabhane
Copy link
Member

Got it, thanks!

@Jag96 Jag96 removed the n6-hold label Oct 18, 2021
@Jag96
Copy link
Contributor

Jag96 commented Oct 18, 2021

Removed the n6-hold label and created an internal ticket to get the repo forked. @rushatgabhane once the repo is forked I'll post the URL here so you can create your PR against it!

@Jag96
Copy link
Contributor

Jag96 commented Oct 19, 2021

@rushatgabhane the fork can be located here: https://github.com/Expensify/react-native-haptic-feedback. Please create your PR there! You can then create a draft PR in Expensify/App that has the changes required and points to your latest commit on your Expensify/react-native-haptic-feedback PR so we can test the changes that way.

@rushatgabhane
Copy link
Member

Hi Joe!
My PR for adding new haptic methods got merged into junina-de/react-native-haptic-feedback.

So I created a new draft PR (5960) which uses it, and fixes this issue.

Expensify's fork of rn-haptic-feedback isn't needed now.

@Jag96 Jag96 added the Reviewing Has a PR in review label Oct 26, 2021
@Jag96
Copy link
Contributor

Jag96 commented Oct 26, 2021

@rushatgabhane sounds good, once the Expensify/App PR is merged I'll go ahead and remove the unneeded repo

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 2, 2021
@botify
Copy link

botify commented Nov 2, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.12-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2021-11-09. 🎊

@botify botify changed the title Reduce the haptic feedback vibration to a short bip [HOLD for payment 2021-11-09] Reduce the haptic feedback vibration to a short bip Nov 2, 2021
@Jag96
Copy link
Contributor

Jag96 commented Nov 11, 2021

Paid, including n6 hold bonus!

@Jag96 Jag96 closed this as completed Nov 11, 2021
@rushatgabhane
Copy link
Member

Thanks a ton!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests