💲[Native Checkout] Stepper haptics #711

dusi · 2019-06-18T21:18:54Z

📲 What

Adds haptic feedback to the amount stepper.

Based on design specs:

We're using selection feedback for incrementing/decrementing
We're using notification feedback for reaching minimum/maximum value

🤔 Why

Providing great UX to our users

🛠 How

The logic is pretty simple, when the stepper reaches its minimum or maximum value we trigger UIFeedbackGenerator.ksr_generateWarningFeedback(), when the stepper increments/decrements within its limited we trigger UIFeedbackGenerator.ksr_generateSelectionFeedback() (iOS does the rest for us.

Please note that big chunk of work was actually refactoring our haptic feedback mechanism in order for it to being more testable and remove some duplicates.

Previously we used UIButton extension proxy to trigger feedback on UIFeedbackGenerator...some of those proxies were not even used so I've removed them and hopefully this will make things simpler.

So where we would previously use:

self.button.ksr_success()

we now don't need to reference self (mostly in closures) and can directly call

UIFeedbackGenerator.ksr_generateSuccess()

in addition to that this is going through AppEnvironment.current which allows us to actually observe whether the correct feedback was triggered or not and allows us to mock it.

✅ Acceptance criteria

You'll notice that currently we hardcode min, max and initial value (this is for the testing purposes and will be fixed in future work...right now we're using 10 for minimum stepper value, 20 for maximum stepper value and 15 for default stepper value).

Incrementing or decrementing by either holding - or + stepper button triggers selection haptic feedback (very subtle)
Reaching minimum or maximum triggers notification haptic feedback (more prominent)

⏰ TODO

This PR does not include the following (which will be separate pieces of work)

Dynamically set min, max and initial values based on project setting
Dynamically set incrementation step value (currently set to 1)
Allow stepper to interoperate with textfield input
Limit textfield input to digits and decimal separators only (based on locale)

dusi · 2019-06-18T21:20:05Z

Kickstarter-iOS/Views/Cells/DiscoveryPostcardCell.swift

@@ -257,21 +257,15 @@ internal final class DiscoveryPostcardCell: UITableViewCell, ValueCell {

    self.watchProjectViewModel.outputs.generateImpactFeedback
      .observeForUI()
-      .observeValues { [weak self] in


We now no longer need self reference...in the future this could also be refactored into a side effect so there would not be a need for generateImpactFeedback signal from the VM. I identified that this would make the changes much bigger and decided against doing that.

dusi · 2019-06-18T21:25:35Z

Kickstarter-iOS/Views/Controllers/ProjectNavBarViewController.swift

@@ -122,21 +122,15 @@ public final class ProjectNavBarViewController: UIViewController {

    self.watchProjectViewModel.outputs.generateImpactFeedback
      .observeForUI()
-      .observeValues { [weak self] in
-        self?.saveButton.generateImpactFeedback(style: .light)


Team notice that this is a thing of past (these proxies were just duplicating things and made a lot of reasoning about our code harder than need be).

So previously the flow would be

button.generateImpactFeedback -> UIFeedbackGenerator.ksr_impact() -> UIImpactFeedbackGenerator(style: style).impactOccurred()`

now we're removing one layer and doing

UIFeedbackGenerator.ksr_generateImpactFeedback() -> AppEnvironment.current.impactFeedbackGenerator.impactOccured

(please not that the naming is now more consistent) & (this also allows us to test things)

Cc: @justinswart @cdolm92 @ifbarrera @Scollaco

After some feedback from Justin it's now even simpler...we only have one publicly available function for each haptic type.

So what was previously this

button.generateImpactFeedback -> UIFeedbackGenerator.ksr_impact() -> UIImpactFeedbackGenerator(style: style).impactOccurred()

is now

generateImpactFeedback()

dusi · 2019-06-18T21:26:00Z

Library/Environment.swift

@@ -73,13 +73,19 @@ public struct Environment {
  /// The current set of launched countries for Kickstarter.
  public let launchedCountries: LaunchedCountries

+  /// Impact haptic feedback with light style
+  public let lightImpactFeedbackGenerator: UIImpactFeedbackGeneratorType


By making these injectable we can test haptics 🙌

dusi · 2019-06-18T21:26:48Z

Library/UIButton+HapticFeedback.swift

-    UIFeedbackGenerator.ksr_selection()
-  }
-
-  public func generateSuccessFeedback() {


I believe previously only generateSuccessFeedback was used through proxy so the other 2 functions were unused

dusi · 2019-06-18T21:27:03Z

Library/UIFeedbackGenerator+Extensions.swift

@@ -1,15 +1,19 @@
 import UIKit

 extension UIFeedbackGenerator {
-  public static func ksr_success() {
-    UINotificationFeedbackGenerator().notificationOccurred(.success)
+  public static func ksr_generateImpactFeedback(style _: UIImpactFeedbackGenerator.FeedbackStyle = .light) {


Sorting alphabetically + more consistent naming

dusi · 2019-06-18T21:27:29Z

Library/ViewModels/PledgeAmountCellViewModel.swift


    self.currency = project
      .map { currencySymbol(forCountry: $0.country).trimmed() }
+
+    let minAndMax = Signal.combineLatest(project, reward)


Hardcoding min, max and initialValue for now..will be fixed in future PRs

justinswart

Cool! Just some questions/comments.

justinswart · 2019-06-18T22:17:04Z

Library/Environment.swift

@@ -117,11 +126,14 @@ public struct Environment {
    koala: Koala = Koala(client: KoalaTrackingClient(endpoint: .production)),
    language: Language = Language(languageStrings: Locale.preferredLanguages) ?? Language.en,
    launchedCountries: LaunchedCountries = .init(),
+    lightImpactFeedbackGenerator: UIImpactFeedbackGeneratorType = UIImpactFeedbackGenerator(style: .light),


Given that at the callsite we access all of these through an extension on UIFeedbackGenerator, would it not be less to add to the Environment (and less to mock) if we just added a type like UIFeedbackGeneratorType?

I guess one other question I have is - does it matter if these generator instances exist for the lifetime of the app? I'm guessing they're pretty lightweight like tap gestures and it doesn't matter too much, but perhaps if we moved the instantiation of them into the type that we create then they'd only exist when we need them.

Lastly - I've seen in the documentation that we're supposed to call prepare(), I don't think we were doing that before, but perhaps it's a good idea to add it.

justinswart · 2019-06-18T22:19:26Z

Library/TestHelpers/XCTestCase+AppEnvironment.swift

@@ -4,6 +4,7 @@ import KsApi
 import ReactiveSwift
 import XCTest

+// swiftlint:disable line_length
 extension XCTestCase {
  // Pushes an environment onto the stack, executes a closure, and then pops the environment from the stack.
  func withEnvironment(_ env: Environment, body: () -> Void) {


When adding stuff to Environment be sure to also carry the arguments through to the pushEnvironment and replaceCurrentEnvironment functions in Library/AppEnvironment.swift too 👍

dusi · 2019-06-20T20:50:38Z

Cartfile

@@ -1,7 +1,7 @@
 ### Internal

 github "kickstarter/Kickstarter-Prelude" "c96c90b026d45839724e827f881dcba3ec812725"
-github "kickstarter/Kickstarter-ReactiveExtensions" "665b5cd4941e54e5d120b80218f62d6ae0e66dac"
+github "kickstarter/Kickstarter-ReactiveExtensions" "1b12cd6236aaace5e90a75332309c15c3585e162"


Please also review ReactiveExtensions updates

dusi · 2019-06-20T20:52:04Z

Kickstarter-iOS/Library/SharedFunctions.swift

@@ -1,6 +1,38 @@
 import Library
 import UIKit

+// MARK: - Haptic feedback
+
+func generateImpactFeedback(


Convenience functions should now be unified place to trigger with the following improvements:

we call prepare to get the HW ready for haptic feedback

we do not store reference to the haptic feedback generator (so the system releases it right after)

dusi · 2019-06-20T20:52:24Z

Kickstarter-iOS/Library/SharedFunctionsTests.swift

@@ -3,6 +3,42 @@
 import XCTest

 internal final class SharedFunctionsTests: XCTestCase {
+  func testGenerateImpactFeedback() {


Tests to make sure that we call both prepare and the right haptic function

dusi · 2019-06-20T20:52:41Z

Kickstarter-iOS/Library/UIFeedbackGeneratorType.swift

@@ -0,0 +1,31 @@
+import UIKit


Protocol that allows us to mock and test our haptic feedback

dusi · 2019-06-20T20:53:13Z

Kickstarter-iOS/TestHelpers/MockAppEnvironment.swift

@@ -0,0 +1,27 @@
+import Foundation


Previously this was part of the test file that I touched, so I've extracted this since I was doing the same work for MockFeedbackGenerator (keeps the test file small and readable)

dusi · 2019-06-20T20:54:05Z

Library/ViewModels/WatchProjectViewModel.swift

@@ -21,7 +21,7 @@ public protocol WatchProjectViewModelOutputs {
  var generateSelectionFeedback: Signal<(), Never> { get }

  /// Emits when haptic feedback should be generated
-  var generateSuccessFeedback: Signal<(), Never> { get }
+  var generateNotificationSuccessFeedback: Signal<(), Never> { get }


Renamed in order to be consistent with our haptic feedback convenience functions

ksr-ci-bot · 2019-06-20T20:54:37Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

justinswart · 2019-06-20T23:13:37Z

Kickstarter-iOS/Library/SharedFunctions.swift

+  feedbackGenerator.impactOccurred()
+}
+
+func generateNotificationSuccessFeedback(


Any concern with these not being prefixed ksr_?

Since those are not extension functions or anything like that I went with simplicity

justinswart · 2019-06-20T23:18:11Z

Kickstarter-iOS/Library/SharedFunctionsTests.swift

+    XCTAssertTrue(mockFeedbackGenerator.impactOccurredWasCalled)
+  }
+
+  func testGEnerateNotificationSuccessFeedback() {


GE -> Ge

Next time if you use Github's suggestion I can update it right from this PR

justinswart

DO ITLIVE