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

πŸ’²[Native Checkout] Shipping Picker - Navigation Workflow - 1/4 #738

Merged
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
61304be
Rename button
dusi Jun 27, 2019
679e925
Hook the button up to its action
dusi Jun 27, 2019
984c9dc
Merge branch 'feature-native-checkout' into feature-native-checkout-s…
dusi Jun 28, 2019
0b5c286
Fit target action on one line
dusi Jul 2, 2019
7acbf60
Refactor shipping rules logic
dusi Jul 3, 2019
f4b9773
Revert MockService changes
dusi Jul 4, 2019
b9ba115
Hook up selection of a shipping rule to presenting a picker
dusi Jul 4, 2019
81a80f6
Merge branch 'feature-native-checkout' into feature-native-checkout-s…
dusi Jul 4, 2019
4d95e88
Merge branch 'feature-native-checkout-shipping-location-vm-refactor' …
dusi Jul 4, 2019
bc80e17
Merge branch 'feature-native-checkout' into feature-native-checkout-s…
dusi Jul 4, 2019
7393789
Merge branch 'feature-native-checkout-shipping-location-vm-refactor' …
dusi Jul 4, 2019
8e464e5
Rename button signal
dusi Jul 5, 2019
7c412a6
Rename a property
dusi Jul 5, 2019
c4ecb5b
Add comments
dusi Jul 5, 2019
595d125
Extract tuple to a private function for allowing point free assignment
dusi Jul 5, 2019
b9aa1fa
Explicitly state masked corners type to help the compiler
dusi Jul 5, 2019
5579fac
Merge branch 'feature-native-checkout' into feature-native-checkout-s…
dusi Jul 5, 2019
8f50d34
Merge branch 'feature-native-checkout-shipping-location-vm-refactor' …
dusi Jul 5, 2019
edffb9f
Rename view controller
dusi Jul 5, 2019
1851a3d
Extract offset value to a private enum constant
dusi Jul 5, 2019
3d6fc62
Merge branch 'feature-native-checkout' into feature-native-checkout-s…
dusi Jul 5, 2019
67bdad5
Rename delegate method
dusi Jul 8, 2019
570ed94
Rename signals
dusi Jul 8, 2019
390f15c
Update top offset
dusi Jul 8, 2019
d02aa04
Rename output signal
dusi Jul 9, 2019
6f0d173
Merge branch 'feature-native-checkout' into feature-native-checkout-s…
dusi Jul 9, 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
11 changes: 10 additions & 1 deletion Kickstarter-iOS/Views/Cells/PledgeShippingLocationCell.swift
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ final class PledgeShippingLocationCell: UITableViewCell, ValueCell {

self.amountLabel.rac.attributedText = self.viewModel.outputs.amountAttributedText
self.shippingLocationButton.rac.title = self.viewModel.outputs.shippingLocationButtonTitle

self.viewModel.outputs.shippingLocationSelected
.observeForUI()
.observeValues { [weak self] shippingRule in
guard let self = self else { return }
self.delegate?.pledgeShippingCell(self, didSelectShippingRule: shippingRule)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing as ShippingRule is a type that explains this at the callsite, we can actually make this:

self.delegate?.pledgeShippingCell(self, didSelect: shippingRule)

Copy link
Contributor

Choose a reason for hiding this comment

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

See later comment, I now realize exactly what the delegate is doing, it's really just presenting the rules, the actual selection will happen elsewhere.

}
}

// MARK: - Configuration
Expand All @@ -115,7 +122,9 @@ final class PledgeShippingLocationCell: UITableViewCell, ValueCell {

// MARK: - Actions

@objc func shippingLocationButtonTapped(_: UIButton) {}
@objc func shippingLocationButtonTapped(_: UIButton) {
self.viewModel.inputs.shippingLocationButtonTapped()
}
}

// MARK: - Styles
Expand Down
50 changes: 36 additions & 14 deletions Kickstarter-iOS/Views/Controllers/PledgeTableViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ import Library
import Prelude
import UIKit

private enum Layout {
enum Sheet {
static let offset: CGFloat = 200
Copy link
Contributor

Choose a reason for hiding this comment

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

This fine for different device sizes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it looks like we need enough space to still show the shipping location cell behind the modal.

}
}

class PledgeTableViewController: UITableViewController {
// MARK: - Properties

Expand Down Expand Up @@ -67,6 +73,14 @@ class PledgeTableViewController: UITableViewController {
}
}

self.viewModel.outputs.presentShippingRules
.observeForUI()
.observeValues { [weak self] project, shippingRules, selectedShippingRule in
self?.presentShippingRules(
project, shippingRules: shippingRules, selectedShippingRule: selectedShippingRule
Copy link
Contributor

Choose a reason for hiding this comment

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

The argument naming here is fine because selectedShippingRule is a noun in this case, if we changed this to selected: even though we have the ShippingRule type it would change the meaning to be an action.

)
}

self.viewModel.outputs.configureShippingLocationCellWithData
.observeForUI()
.observeValues { [weak self] isLoading, project, selectedShippingRule in
Expand All @@ -82,12 +96,6 @@ class PledgeTableViewController: UITableViewController {
}
}

// MARK: - Actions

@objc func dismissKeyboard() {
self.tableView.endEditing(true)
}

// MARK: - UITableViewDelegate

override func tableView(_ tableView: UITableView, viewForFooterInSection section: Int) -> UIView? {
Expand Down Expand Up @@ -118,10 +126,24 @@ class PledgeTableViewController: UITableViewController {

// MARK: - Actions

@objc func dismissKeyboard() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Little bit of reordering #boyScoutRule

self.tableView.endEditing(true)
}

private func presentHelpWebViewController(with helpType: HelpType) {
let vc = HelpWebViewController.configuredWith(helpType: helpType)
let nav = UINavigationController(rootViewController: vc)
self.present(nav, animated: true, completion: nil)
let nc = UINavigationController(rootViewController: vc)
self.present(nc, animated: true)
}

private func presentShippingRules(
_: Project, shippingRules _: [ShippingRule], selectedShippingRule _: ShippingRule
) {
let vc = UIViewController()
vc.view.backgroundColor = UIColor.cyan
let nc = UINavigationController(rootViewController: vc)
let sheetVC = SheetOverlayViewController(child: nc, offset: Layout.Sheet.offset)
self.present(sheetVC, animated: true)
}
}

Expand All @@ -131,9 +153,9 @@ extension PledgeTableViewController: PledgeDescriptionCellDelegate {
}
}

extension PledgeTableViewController: PledgeSummaryCellDelegate {
internal func pledgeSummaryCell(_: PledgeSummaryCell, didOpen helpType: HelpType) {
self.presentHelpWebViewController(with: helpType)
extension PledgeTableViewController: PledgeAmountCellDelegate {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delegate conformance sorted alphabetically now

func pledgeAmountCell(_: PledgeAmountCell, didUpdateAmount amount: Double) {
self.viewModel.inputs.pledgeAmountDidUpdate(to: amount)
}
}

Expand All @@ -143,9 +165,9 @@ extension PledgeTableViewController: PledgeShippingLocationCellDelegate {
}
}

extension PledgeTableViewController: PledgeAmountCellDelegate {
func pledgeAmountCell(_: PledgeAmountCell, didUpdateAmount amount: Double) {
self.viewModel.inputs.pledgeAmountDidUpdate(to: amount)
extension PledgeTableViewController: PledgeSummaryCellDelegate {
internal func pledgeSummaryCell(_: PledgeSummaryCell, didOpen helpType: HelpType) {
self.presentHelpWebViewController(with: helpType)
}
}

Expand Down
12 changes: 12 additions & 0 deletions Library/ViewModels/PledgeShippingLocationCellViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import ReactiveSwift

public protocol PledgeShippingLocationCellViewModelInputs {
func configureWith(isLoading: Bool, project: Project, selectedShippingRule: ShippingRule?)
func shippingLocationButtonTapped()
}

public protocol PledgeShippingLocationCellViewModelOutputs {
var amountAttributedText: Signal<NSAttributedString, Never> { get }
var shippingLocationButtonTitle: Signal<String, Never> { get }
var shippingLocationSelected: Signal<ShippingRule, Never> { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

Not selectedShippingLocation ?

}

public protocol PledgeShippingLocationCellViewModelType {
Expand All @@ -32,15 +34,25 @@ public final class PledgeShippingLocationCellViewModel: PledgeShippingLocationCe

self.shippingLocationButtonTitle = projectAndSelectedShippingRule
.map { _, selectedShippingRule in selectedShippingRule.location.localizedName }

self.shippingLocationSelected = projectAndSelectedShippingRule
.map(second)
.takeWhen(self.shippingLocationButtonTappedProperty.signal)
}

private let configDataProperty = MutableProperty<(Bool, Project, ShippingRule?)?>(nil)
public func configureWith(isLoading: Bool, project: Project, selectedShippingRule: ShippingRule?) {
self.configDataProperty.value = (isLoading, project, selectedShippingRule)
}

private let shippingLocationButtonTappedProperty = MutableProperty(())
public func shippingLocationButtonTapped() {
self.shippingLocationButtonTappedProperty.value = ()
}

public let amountAttributedText: Signal<NSAttributedString, Never>
public let shippingLocationButtonTitle: Signal<String, Never>
public let shippingLocationSelected: Signal<ShippingRule, Never>

public var inputs: PledgeShippingLocationCellViewModelInputs { return self }
public var outputs: PledgeShippingLocationCellViewModelOutputs { return self }
Expand Down
23 changes: 18 additions & 5 deletions Library/ViewModels/PledgeShippingLocationCellViewModelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ final class PledgeShippingLocationCellViewModelTests: TestCase {
private let vm: PledgeShippingLocationCellViewModelType = PledgeShippingLocationCellViewModel()

private let amountAttributedText = TestObserver<NSAttributedString, Never>()
private let shippingLocation = TestObserver<String, Never>()
private let shippingLocationButtonTitle = TestObserver<String, Never>()
private let shippingLocationSelected = TestObserver<ShippingRule, Never>()

override func setUp() {
super.setUp()

self.vm.outputs.amountAttributedText.observe(self.amountAttributedText.observer)
self.vm.outputs.shippingLocationButtonTitle.observe(self.shippingLocation.observer)
self.vm.outputs.shippingLocationButtonTitle.observe(self.shippingLocationButtonTitle.observer)
self.vm.outputs.shippingLocationSelected.observe(self.shippingLocationSelected.observer)
}

func testAmountAttributedText() {
Expand All @@ -35,11 +37,22 @@ final class PledgeShippingLocationCellViewModelTests: TestCase {
self.amountAttributedText.assertValues([expectedAttributedString])
}

func testShippingLocation() {
func testShippingLocationButtonTitle() {
self.vm.inputs.configureWith(isLoading: false, project: .template, selectedShippingRule: nil)
self.shippingLocation.assertDidNotEmitValue()
self.shippingLocationButtonTitle.assertDidNotEmitValue()

self.vm.inputs.configureWith(isLoading: false, project: .template, selectedShippingRule: .template)
self.shippingLocation.assertValues(["Brooklyn, NY"])
self.shippingLocationButtonTitle.assertValues(["Brooklyn, NY"])
}

func testShippingLocationSelected() {
self.vm.inputs.shippingLocationButtonTapped()
self.shippingLocationSelected.assertDidNotEmitValue()

let shippingRule = ShippingRule.template

self.vm.inputs.configureWith(isLoading: false, project: .template, selectedShippingRule: shippingRule)
self.vm.inputs.shippingLocationButtonTapped()
self.shippingLocationSelected.assertValues([shippingRule])
}
}
23 changes: 14 additions & 9 deletions Library/ViewModels/PledgeViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public protocol PledgeViewModelOutputs {
recycled it will be reloaded with its most recent data.
*/
var pledgeViewDataAndReload: Signal<(PledgeViewData, Bool), Never> { get }
var presentShippingRules: Signal<[ShippingRule], Never> { get }
var presentShippingRules: Signal<(Project, [ShippingRule], ShippingRule), Never> { get }
var shippingRulesError: Signal<String, Never> { get }
}

Expand Down Expand Up @@ -74,15 +74,20 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
.materialize()
}

self.presentShippingRules = shippingRulesEvent.values()

let selectedShippingRule = Signal.merge(
let defaultShippingRule = Signal.merge(
self.viewDidLoadProperty.signal.mapConst(nil),
self.presentShippingRules.map(defaultShippingRule(fromShippingRules:))
shippingRulesEvent.values().map(defaultShippingRule(fromShippingRules:))
)

self.presentShippingRules = Signal.combineLatest(
project,
shippingRulesEvent.values(),
defaultShippingRule.skipNil()
)
.takeWhen(self.shippingRuleSignal.signal)
Copy link
Contributor

Choose a reason for hiding this comment

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

shippingRuleSignal doesn't seem like the right name here, this probably just points to our delegate naming perhaps not being explanatory enough. I think we'd want this to indicate that the selectShippingRuleButton was tapped and that the currently selected ShippingRule was passed in.

So perhaps the delegate should be:

func pledgeShippingCellWillPresentShippingRules(_: PledgeShippingLocationCell, selectedShippingRule rule: ShippingRule)
// or
func pledgeShippingCellPresentedShippingRules(_: PledgeShippingLocationCell, selectedShippingRule rule: ShippingRule)

I think we can then base the view model input on that and it should improve the signal naming.


let shippingAmount = Signal.merge(
selectedShippingRule.skipNil().map { $0.cost },
defaultShippingRule.skipNil().map { $0.cost },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming as this is the default value..when we allow selection we will add another signal replacing this.

self.shippingRuleSignal.map { $0.cost },
projectAndReward.mapConst(0)
)
Expand All @@ -102,7 +107,7 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
let pledgeTotal = Signal.combineLatest(pledgeAmount, shippingAmount).map(+)

let data = Signal
.combineLatest(project, reward, isLoggedIn, isShippingLoading, selectedShippingRule, pledgeTotal)
.combineLatest(project, reward, isLoggedIn, isShippingLoading, defaultShippingRule, pledgeTotal)
.map(pledgeViewData(with:reward:isLoggedIn:isShippingLoading:selectedShippingRule:pledgeTotal:))

self.pledgeViewDataAndReload = Signal.merge(
Expand All @@ -120,7 +125,7 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge

let isShippingLoadingAndSelectedShippingRule = Signal.combineLatest(
isShippingLoading,
selectedShippingRule
defaultShippingRule
)

self.configureShippingLocationCellWithData = updatedData
Expand Down Expand Up @@ -154,7 +159,7 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
public let configureShippingLocationCellWithData: Signal<(Bool, Project, ShippingRule?), Never>
public let configureSummaryCellWithData: Signal<(Project, Double), Never>
public let pledgeViewDataAndReload: Signal<(PledgeViewData, Bool), Never>
public let presentShippingRules: Signal<[ShippingRule], Never>
public let presentShippingRules: Signal<(Project, [ShippingRule], ShippingRule), Never>
public let shippingRulesError: Signal<String, Never>

public var inputs: PledgeViewModelInputs { return self }
Expand Down
35 changes: 22 additions & 13 deletions Library/ViewModels/PledgeViewModelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ final class PledgeViewModelTests: TestCase {
private let pledgeViewDataAndReloadSelectedShippingRule = TestObserver<ShippingRule?, Never>()
private let pledgeViewDataAndReloadTotal = TestObserver<Double, Never>()

private let presentShippingRules = TestObserver<[ShippingRule], Never>()
private let presentShippingRulesProject = TestObserver<Project, Never>()
private let presentShippingRulesShippingRules = TestObserver<[ShippingRule], Never>()
private let presentShippingRulesSelectedShippingRule = TestObserver<ShippingRule, Never>()

private let shippingRulesError = TestObserver<String, Never>()

override func setUp() {
Expand All @@ -55,7 +58,10 @@ final class PledgeViewModelTests: TestCase {
self.vm.outputs.pledgeViewDataAndReload.map(first).map { $0.3 }.map { $0.2 }.observe(self.pledgeViewDataAndReloadSelectedShippingRule.observer)
self.vm.outputs.pledgeViewDataAndReload.map(first).map { $0.4 }.observe(self.pledgeViewDataAndReloadTotal.observer)

self.vm.outputs.presentShippingRules.observe(self.presentShippingRules.observer)
self.vm.outputs.presentShippingRules.map(first).observe(self.presentShippingRulesProject.observer)
self.vm.outputs.presentShippingRules.map(second).observe(self.presentShippingRulesShippingRules.observer)
self.vm.outputs.presentShippingRules.map(third).observe(self.presentShippingRulesSelectedShippingRule.observer)

self.vm.outputs.shippingRulesError.observe(self.shippingRulesError.observer)
}

Expand Down Expand Up @@ -405,26 +411,29 @@ final class PledgeViewModelTests: TestCase {
}

func testPresentShippingRules() {
let shippingRules = [.usa, .canada, .greatBritain, .australia]
.enumerated()
.map { idx, location in
.template
|> ShippingRule.lens.location .~ location
|> ShippingRule.lens.cost .~ Double(idx + 1 * 10)
}
let shippingRules: [ShippingRule] = [.template, .template, .template]

withEnvironment(apiService: MockService(fetchShippingRulesResult: .success(shippingRules))) {
let project = Project.template
let reward = Reward.template
|> Reward.lens.shipping.enabled .~ true

self.vm.inputs.viewDidLoad()
self.vm.inputs.configureWith(project: .template, reward: reward)

self.presentShippingRules.assertValues([[]])
self.vm.inputs.configureWith(project: project, reward: reward)

self.scheduler.advance()

self.presentShippingRules.assertValues([[], shippingRules])
self.presentShippingRulesProject.assertDidNotEmitValue()
self.presentShippingRulesShippingRules.assertDidNotEmitValue()
self.presentShippingRulesSelectedShippingRule.assertDidNotEmitValue()

let shippingRule = shippingRules[0]

self.vm.inputs.shippingRuleDidUpdate(to: shippingRule)

self.presentShippingRulesProject.assertValues([project])
self.presentShippingRulesShippingRules.assertValues([shippingRules])
self.presentShippingRulesSelectedShippingRule.assertValues([shippingRule])
}
}

Expand Down