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.

Sign up for GitHub

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

💲[Native Checkout] Stepper haptics #711

Merged
merged 7 commits into from
Jun 21, 2019
Merged

💲[Native Checkout] Stepper haptics #711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d567707
Refactor haptic feedback to being more testable
dusi Jun 18, 2019
51518b5
Add haptic feedback for the amount stepper
dusi Jun 18, 2019
34c14f9
Update reactive extensions dependency
dusi Jun 18, 2019
1427503
Use UIStepper reactive extensions
dusi Jun 18, 2019
efce1e6
Reorder alphabetically
dusi Jun 18, 2019
fa57f81
Refactor feedback generators
dusi Jun 20, 2019
a879bf3
Fix capitalization typo
dusi Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cartfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
### Internal

github "kickstarter/Kickstarter-Prelude" "c96c90b026d45839724e827f881dcba3ec812725"
github "kickstarter/Kickstarter-ReactiveExtensions" "665b5cd4941e54e5d120b80218f62d6ae0e66dac"
github "kickstarter/Kickstarter-ReactiveExtensions" "1b12cd6236aaace5e90a75332309c15c3585e162"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please also review ReactiveExtensions updates


### 3rd Party

Expand Down
2 changes: 1 addition & 1 deletion Cartfile.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ github "Alamofire/Alamofire" "5.0.0-beta.6"
github "Alamofire/AlamofireImage" "c41f8b0acfbb3180fe045df73596e4332c338633"
github "ReactiveCocoa/ReactiveSwift" "6.0.0"
github "kickstarter/Kickstarter-Prelude" "c96c90b026d45839724e827f881dcba3ec812725"
github "kickstarter/Kickstarter-ReactiveExtensions" "665b5cd4941e54e5d120b80218f62d6ae0e66dac"
github "kickstarter/Kickstarter-ReactiveExtensions" "1b12cd6236aaace5e90a75332309c15c3585e162"
github "stripe/stripe-ios" "v13.2.0"
github "thoughtbot/Argo" "39f06f089d25c111444e5a85eef64586e54756ac"
github "thoughtbot/Curry" "b6bf27ec9d711f607a8c7da9ca69ee9eaa201a22"
Expand Down
32 changes: 32 additions & 0 deletions Kickstarter-iOS/Library/SharedFunctions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,38 @@
import Library
import UIKit

// MARK: - Haptic feedback

func generateImpactFeedback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

feedbackGenerator: UIImpactFeedbackGeneratorType = UIImpactFeedbackGenerator(style: .light)
) {
feedbackGenerator.prepare()
feedbackGenerator.impactOccurred()
}

func generateNotificationSuccessFeedback(
Copy link
Contributor

Choose a reason for hiding this comment

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

Any concern with these not being prefixed ksr_?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

feedbackGenerator: UINotificationFeedbackGeneratorType = UINotificationFeedbackGenerator()
) {
feedbackGenerator.prepare()
feedbackGenerator.notificationOccurred(.success)
}

func generateNotificationWarningFeedback(
feedbackGenerator: UINotificationFeedbackGeneratorType = UINotificationFeedbackGenerator()
) {
feedbackGenerator.prepare()
feedbackGenerator.notificationOccurred(.warning)
}

func generateSelectionFeedback(
feedbackGenerator: UISelectionFeedbackGeneratorType = UISelectionFeedbackGenerator()
) {
feedbackGenerator.prepare()
feedbackGenerator.selectionChanged()
}

// MARK: - Login workflow

public func logoutAndDismiss(
viewController: UIViewController,
appEnvironment: AppEnvironmentType.Type = AppEnvironment.self,
Expand Down
60 changes: 36 additions & 24 deletions Kickstarter-iOS/Library/SharedFunctionsTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,42 @@
import XCTest

internal final class SharedFunctionsTests: XCTestCase {
func testGenerateImpactFeedback() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let mockFeedbackGenerator = MockImpactFeedbackGenerator()

generateImpactFeedback(feedbackGenerator: mockFeedbackGenerator)

XCTAssertTrue(mockFeedbackGenerator.prepareWasCalled)
XCTAssertTrue(mockFeedbackGenerator.impactOccurredWasCalled)
}

func testGenerateNotificationSuccessFeedback() {
let mockFeedbackGenerator = MockNotificationFeedbackGenerator()

generateNotificationSuccessFeedback(feedbackGenerator: mockFeedbackGenerator)

XCTAssertTrue(mockFeedbackGenerator.prepareWasCalled)
XCTAssertTrue(mockFeedbackGenerator.notificationOccurredWasCalled)
}

func testGenerateNotificationWarningFeedback() {
let mockFeedbackGenerator = MockNotificationFeedbackGenerator()

generateNotificationWarningFeedback(feedbackGenerator: mockFeedbackGenerator)

XCTAssertTrue(mockFeedbackGenerator.prepareWasCalled)
XCTAssertTrue(mockFeedbackGenerator.notificationOccurredWasCalled)
}

func testGenerateSelectionFeedback() {
let mockFeedbackGenerator = MockSelectionFeedbackGenerator()

generateSelectionFeedback(feedbackGenerator: mockFeedbackGenerator)

XCTAssertTrue(mockFeedbackGenerator.prepareWasCalled)
XCTAssertTrue(mockFeedbackGenerator.selectionChangedWasCalled)
}

func testLogoutAndDismiss() {
let mockAppEnvironment = MockAppEnvironment.self
let mockPushNotificationDialog = MockPushNotificationDialog.self
Expand All @@ -20,27 +56,3 @@ internal final class SharedFunctionsTests: XCTestCase {
XCTAssertTrue(mockViewController.dismissAnimatedWasCalled)
}
}

private struct MockAppEnvironment: AppEnvironmentType {
static var logoutWasCalled = false

static func logout() {
self.logoutWasCalled = true
}
}

private struct MockPushNotificationDialog: PushNotificationDialogType {
static var resetAllContextsWasCalled = false

static func resetAllContexts() {
self.resetAllContextsWasCalled = true
}
}

private class MockViewController: UIViewController {
var dismissAnimatedWasCalled = false

override func dismiss(animated _: Bool, completion _: (() -> Void)? = nil) {
self.dismissAnimatedWasCalled = true
}
}
31 changes: 31 additions & 0 deletions Kickstarter-iOS/Library/UIFeedbackGeneratorType.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import UIKit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Protocol that allows us to mock and test our haptic feedback


// MARK: - UIFeedbackGenerator

protocol UIFeedbackGeneratorType {
func prepare()
}

// MARK: - UIImpactFeedbackGeneratorType

protocol UIImpactFeedbackGeneratorType: UIFeedbackGeneratorType {
func impactOccurred()
}

extension UIImpactFeedbackGenerator: UIImpactFeedbackGeneratorType {}

// MARK: - UINotificationFeedbackGeneratorType

protocol UINotificationFeedbackGeneratorType: UIFeedbackGeneratorType {
func notificationOccurred(_ notificationType: UINotificationFeedbackGenerator.FeedbackType)
}

extension UINotificationFeedbackGenerator: UINotificationFeedbackGeneratorType {}

// MARK: - UISelectionFeedbackGeneratorType

protocol UISelectionFeedbackGeneratorType: UIFeedbackGeneratorType {
func selectionChanged()
}

extension UISelectionFeedbackGenerator: UISelectionFeedbackGeneratorType {}
27 changes: 27 additions & 0 deletions Kickstarter-iOS/TestHelpers/MockAppEnvironment.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import Foundation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

@testable import Library
import UIKit

struct MockAppEnvironment: AppEnvironmentType {
static var logoutWasCalled = false

static func logout() {
self.logoutWasCalled = true
}
}

struct MockPushNotificationDialog: PushNotificationDialogType {
static var resetAllContextsWasCalled = false

static func resetAllContexts() {
self.resetAllContextsWasCalled = true
}
}

class MockViewController: UIViewController {
var dismissAnimatedWasCalled = false

override func dismiss(animated _: Bool, completion _: (() -> Void)? = nil) {
self.dismissAnimatedWasCalled = true
}
}
42 changes: 42 additions & 0 deletions Kickstarter-iOS/TestHelpers/MockFeedbackGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import Foundation
@testable import Kickstarter_Framework
import UIKit

class MockImpactFeedbackGenerator: UIImpactFeedbackGeneratorType {
var prepareWasCalled = false
var impactOccurredWasCalled = false

func prepare() {
self.prepareWasCalled = true
}

func impactOccurred() {
self.impactOccurredWasCalled = true
}
}

class MockNotificationFeedbackGenerator: UINotificationFeedbackGeneratorType {
var prepareWasCalled = false
var notificationOccurredWasCalled = false

func prepare() {
self.prepareWasCalled = true
}

func notificationOccurred(_: UINotificationFeedbackGenerator.FeedbackType) {
self.notificationOccurredWasCalled = true
}
}

class MockSelectionFeedbackGenerator: UISelectionFeedbackGeneratorType {
var prepareWasCalled = false
var selectionChangedWasCalled = false

func prepare() {
self.prepareWasCalled = true
}

func selectionChanged() {
self.selectionChangedWasCalled = true
}
}
14 changes: 4 additions & 10 deletions Kickstarter-iOS/Views/Cells/DiscoveryPostcardCell.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,21 +257,15 @@ internal final class DiscoveryPostcardCell: UITableViewCell, ValueCell {

self.watchProjectViewModel.outputs.generateImpactFeedback
.observeForUI()
.observeValues { [weak self] in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

self?.saveButton.generateImpactFeedback(style: .light)
}
.observeValues { _ in generateImpactFeedback() }

self.watchProjectViewModel.outputs.generateSuccessFeedback
self.watchProjectViewModel.outputs.generateNotificationSuccessFeedback
.observeForUI()
.observeValues { [weak self] in
self?.saveButton.generateSuccessFeedback()
}
.observeValues { generateNotificationSuccessFeedback() }

self.watchProjectViewModel.outputs.generateSelectionFeedback
.observeForUI()
.observeValues { [weak self] in
self?.saveButton.generateSelectionFeedback()
}
.observeValues { generateSelectionFeedback() }

self.viewModel.outputs.projectCategoryName
.signal
Expand Down
23 changes: 23 additions & 0 deletions Kickstarter-iOS/Views/Cells/PledgeAmountCell.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ final class PledgeAmountCell: UITableViewCell, ValueCell {

self.spacer.widthAnchor.constraint(greaterThanOrEqualToConstant: Styles.grid(3)).isActive = true

self.stepper.addTarget(
self,
action: #selector(PledgeAmountCell.stepperValueChanged(_:)),
for: .valueChanged
)

self.bindViewModel()
}

Expand Down Expand Up @@ -80,13 +86,30 @@ final class PledgeAmountCell: UITableViewCell, ValueCell {

self.amountInputView.label.rac.text = self.viewModel.outputs.currency
self.amountInputView.textField.rac.text = self.viewModel.outputs.amount
self.stepper.rac.maximumValue = self.viewModel.outputs.stepperMaxValue
self.stepper.rac.minimumValue = self.viewModel.outputs.stepperMinValue
self.stepper.rac.value = self.viewModel.outputs.stepperInitialValue

self.viewModel.outputs.generateSelectionFeedback
.observeForUI()
.observeValues { generateSelectionFeedback() }

self.viewModel.outputs.generateNotificationWarningFeedback
.observeForUI()
.observeValues { generateNotificationWarningFeedback() }
}

// MARK: - Configuration

func configureWith(value: (project: Project, reward: Reward)) {
self.viewModel.inputs.configureWith(project: value.project, reward: value.reward)
}

// MARK: - Actions

@objc func stepperValueChanged(_ stepper: UIStepper) {
self.viewModel.inputs.stepperValueChanged(stepper.value)
}
}

// MARK: - Styles
Expand Down
3 changes: 2 additions & 1 deletion Kickstarter-iOS/Views/Controllers/DeprecatedCheckoutViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ internal final class DeprecatedCheckoutViewController: DeprecatedWebViewControll
self.viewModel.outputs.goToThanks
.observeForControllerAction()
.observeValues { [weak self] project in
UIFeedbackGenerator.ksr_success()
generateNotificationSuccessFeedback()

self?.goToThanks(project: project)
}

Expand Down
3 changes: 2 additions & 1 deletion Kickstarter-iOS/Views/Controllers/DeprecatedRewardPledgeViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,8 @@ internal final class DeprecatedRewardPledgeViewController: UIViewController {
self.viewModel.outputs.goToThanks
.observeForControllerAction()
.observeValues { [weak self] project in
UIFeedbackGenerator.ksr_success()
generateNotificationSuccessFeedback()

self?.goToThanks(project: project)
}

Expand Down
14 changes: 4 additions & 10 deletions Kickstarter-iOS/Views/Controllers/ProjectNavBarViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,21 +122,15 @@ public final class ProjectNavBarViewController: UIViewController {

self.watchProjectViewModel.outputs.generateImpactFeedback
.observeForUI()
.observeValues { [weak self] in
self?.saveButton.generateImpactFeedback(style: .light)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()

}
.observeValues { generateImpactFeedback() }

self.watchProjectViewModel.outputs.generateSuccessFeedback
self.watchProjectViewModel.outputs.generateNotificationSuccessFeedback
.observeForUI()
.observeValues { [weak self] in
self?.saveButton.generateSuccessFeedback()
}
.observeValues { generateNotificationSuccessFeedback() }

self.watchProjectViewModel.outputs.generateSelectionFeedback
.observeForUI()
.observeValues { [weak self] in
self?.saveButton.generateSelectionFeedback()
}
.observeValues { generateSelectionFeedback() }

self.watchProjectViewModel.outputs.showProjectSavedAlert
.observeForControllerAction()
Expand Down
Loading