-
Notifications
You must be signed in to change notification settings - Fork 121
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
Haptics #2611
base: main
Are you sure you want to change the base?
Haptics #2611
Changes from 19 commits
c351bf9
024d2d1
f1a5bbd
2ae6c74
3dae0ef
db68d35
99ba87c
60744b6
cdb1210
9d7ca6f
e2b49da
4c5e8b5
e576860
3b496a9
a6aa73a
07a4abf
e01d73c
ed47d92
abd9aa8
c55030e
7a43c59
aa23971
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import { Haptics } from '@capacitor/haptics' | ||
import { useCallback, useEffect, useMemo, useRef, useState } from 'react' | ||
import { useDispatch } from 'react-redux' | ||
import { editingActionCreator as editing } from '../actions/editing' | ||
|
@@ -41,6 +42,7 @@ const useLongPress = ( | |
clientCoords.current = { x: e.touches?.[0]?.clientX, y: e.touches?.[0]?.clientY } | ||
} | ||
onTouchStart?.() | ||
Haptics.selectionStart() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should go next to
|
||
|
||
// cast Timeout to number for compatibility with clearTimeout | ||
clearTimeout(timerIdRef.current) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
import { Capacitor } from '@capacitor/core' | ||
import { Haptics, ImpactStyle } from '@capacitor/haptics' | ||
import _ from 'lodash' | ||
import { isTouch } from '../browser' | ||
|
||
|
@@ -27,7 +29,6 @@ const fastClick = isTouch | |
const y = e.touches[0].clientY | ||
touchStart = { x, y } | ||
} | ||
|
||
msdewitt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tapDown?.(e) | ||
}, | ||
// cancel tap if touchmove exceeds threshold (e.g. with scrolling or dragging) | ||
|
@@ -42,6 +43,9 @@ const fastClick = isTouch | |
} | ||
}, 16.666), | ||
onTouchEnd: (e: React.TouchEvent) => { | ||
if (Capacitor.isNativePlatform()) { | ||
Haptics.impact({ style: ImpactStyle.Light }) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this should go in the else statement below with |
||
let cancel = !touchStart | ||
|
||
if (touchStart && e.changedTouches.length > 0) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion might be creating our own
haptics
utility which checks forCapacitor.isNativePlatform()
internally so that we don't need to include so many newif
blocks throughout the rest of the code.e.g.:
A reasonable counterargument is that we want to expressly show within the hooks/actions/components that the haptic event is only triggered on native.
I would be okay with leaving this as it is, but slightly prefer creating a utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely want to abstract this out since the condition is present with every Haptics call. It should always NOOP on other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msdewitt #2611 (comment)