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

Set up app for Expo SDK, add haptic feedback for long presses with Expo Haptics #3607

Merged
merged 11 commits into from
Jul 10, 2021

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Jun 16, 2021

NOTE TO REVIEWERS:

Most of the diff comes from changes to package-lock.json, which was updated to support unimodules and expo. While this diff may seem extremely large, I personally think it's useful to allow ourselves to use Expo SDKs, since they provide an easy-to-use and well-supported SDK for a ton of important features that we may want to add later on. You can read more about other Expo SDKs here by using the links in the sidebar.

Details

I previously opened #3586, which added haptic feedback from a haptics library that wasn't as popular as we would have liked, so @marcaaron suggested I use Expo Haptics.

This PR sets up our app with react-native-unimodules, which is needed to use any Expo SDKs, including expo-haptics. I used this guide provided by Expo to set up react-native-unimodules, which is the source of the diffs in any Objective-C, Java, gradle, and Pod files.

It also adds haptic feedback using expo-haptics for any long-presses detected on PressableWithSecondaryInteraction components.

Fixed Issues

Fixes #3576

Tests (must be tested on an actual device and NOT a simulator)

  1. Open any chat with messages.
  2. Long press a message.
  3. Verify that you feel some haptic feedback (a very short and abrupt vibration).

QA Steps

Same as the above tests

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

N/A

@jasperhuangg jasperhuangg changed the title Add haptic feedback for long presses using Expo Haptics Set up app for Expo SDK, add haptic feedback for long presses with Expo Haptics Jun 16, 2021
@jasperhuangg jasperhuangg marked this pull request as ready for review June 16, 2021 06:21
@jasperhuangg jasperhuangg requested a review from a team as a code owner June 16, 2021 06:21
@MelvinBot MelvinBot requested review from HorusGoul and removed request for a team June 16, 2021 06:21
timszot
timszot previously approved these changes Jun 16, 2021
Copy link
Contributor

@timszot timszot left a comment

Choose a reason for hiding this comment

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

LGTM, leaving for others to review

@jasperhuangg jasperhuangg marked this pull request as draft June 17, 2021 02:47
@jasperhuangg
Copy link
Contributor Author

jasperhuangg commented Jun 17, 2021

Just realized that this requires a change to the babel.config for web to work! I'm playing around with it now, will take it off draft-mode once I've figured it out.

@jasperhuangg
Copy link
Contributor Author

jasperhuangg commented Jun 23, 2021

Got the webpack stuff working! Although, I've learned that we won't be able to use a lot of the Expo on web (and they'll actually throw exceptions if we try to).

cc @timszot @marcaaron @Dal-Papa @HorusGoul

@jasperhuangg jasperhuangg marked this pull request as ready for review June 23, 2021 10:57
@Dal-Papa Dal-Papa removed their request for review June 25, 2021 08:45
@marcaaron
Copy link
Contributor

I've learned that we won't be able to use a lot of the Expo on web (and they'll actually throw exceptions if we try to)

@jasperhuangg is this ready for review? I wasn't sure based on the previous comment, but I wouldn't expect to be able to implement haptics on mobile web.

@jasperhuangg
Copy link
Contributor Author

@marcaaron Yup! Sorry if that wasn't clear with my previous comment, this is all good to go 👍

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Works great on Android and doesn't break anything on web. I don't have an iOS device with haptics atm so I can't test that part, but everything builds OK.

I'm requesting changes because I believe we are possibly including packages that we don't need and can minimize the bundle size by removing them. If I'm super off about that lmk, but it would be good to confirm that's not the case.

config/webpack/webpack.common.js Outdated Show resolved Hide resolved
@jasperhuangg
Copy link
Contributor Author

Implemented the suggestions @marcaaron suggested, it still builds fine on Web, and haptics still works great on Android 👍

config/webpack/webpack.common.js Outdated Show resolved Hide resolved
ios/Podfile.lock Outdated Show resolved Hide resolved
@jasperhuangg
Copy link
Contributor Author

@marcaaron Got rid of the unnecessary stuff, should be good for another look!

@jasperhuangg
Copy link
Contributor Author

@HorusGoul @timszot Would love to hear your thoughts on what we've done here! I'm definitely not the most knowledgable about this stuff.

Copy link
Contributor

@HorusGoul HorusGoul left a comment

Choose a reason for hiding this comment

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

Have we considered supporting Mobile Web by leveraging the Vibration API? Only Android browsers support that API, but we could implement it as a progressive enhancement, and when Apple decides to ship the API in Safari, it will work.

@jasperhuangg
Copy link
Contributor Author

Vibration API

Ohh interesting, I haven't heard about this yet. That would definitely be a great idea to get more coverage, interested on hearing other people's thoughts.

@marcaaron
Copy link
Contributor

I'm cool with whatever you want to do here @jasperhuangg.

I personally like the idea of including as many progressive web app features that we can into the mobile web side, but less certain if this is a company wide priority. Feels maybe a bit out of scope to me and we can probably go back and add something like this later.

@jasperhuangg
Copy link
Contributor Author

I'm cool with whatever you want to do here @jasperhuangg.

I personally like the idea of including as many progressive web app features that we can into the mobile web side, but less certain if this is a company wide priority. Feels maybe a bit out of scope to me and we can probably go back and add something like this later.

Sounds good, I think I'll drop a question in #product or #expensify-open-source, and depending on what they say I'll create a new issue for it. In the meantime, I'm gonna go ahead and merge this 👍

@jasperhuangg jasperhuangg merged commit 5066e58 into main Jul 10, 2021
@jasperhuangg jasperhuangg deleted the jasper-expoHapticFeedback branch July 10, 2021 01:08
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@mvtglobally
Copy link

@marcaaron Can you please confirm this is expected to be tested on Mobile devices only for haptic(Vibration) feedback? Desktop and Web will not be easy to validate

@marcaaron
Copy link
Contributor

@mvtglobally That's correct. And also note that this will not work on mobile web either. This is expected.

Only Android and iOS devices with haptic feedback features + running the native app will pass this QA.

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

Mobile - Add haptic feedback when long pressing messages
6 participants