-
Notifications
You must be signed in to change notification settings - Fork 76
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
Support HapticFeedback
on iOS
#1255
Conversation
@@ -426,6 +428,7 @@ internal fun ProvideContainerCompositionLocals( | |||
content: @Composable () -> Unit, | |||
) = with(composeContainer) { | |||
CompositionLocalProvider( | |||
LocalHapticFeedback provides HapticFeedback(), |
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.
It will be recreated on each recomposition
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.
Right, moved it to ComposeContainer
.
// TODO: minor UX improvement, add `prepare()` calls when internal APIs that use HapticFeedback | ||
// to reduce haptic feedback latency | ||
// see https://developer.apple.com/documentation/uikit/uifeedbackgenerator | ||
internal fun HapticFeedback() = object : HapticFeedback { |
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.
Any reason to have fun
+ anonymous object
instead of just a class
?
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.
Not really, looked liked a fine place to use this idiom, but made it CupertionHapticFeedback
class
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.
The code LGTM, but it requires another review with proper testing
override fun performHapticFeedback(hapticFeedbackType: HapticFeedbackType) { | ||
when (hapticFeedbackType) { | ||
HapticFeedbackType.LongPress -> impactGenerator.impactOccurred() | ||
HapticFeedbackType.TextHandleMove -> selectionGenerator.selectionChanged() |
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.
Just to highlight: it looks like haptics will also work for text inputs handle moves, which is not standard for iOS.
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.
Indeed. But it's dictated by ui
library. I don't have a strong opinion on this, let's listen to people feedback about this change. I'd take some actions after the actual feedback.
Proposed Changes
Bridge
HapticFeedback
interface withUIFeedbackGenerator
s, provide the constructed bridge as static composition local.Testing
Test: APIs that request a haptic feedback should trigger one on iOS (text selection, on-demand, etc)
Issues Fixed
Fixes: JetBrains/compose-multiplatform#4598