-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Description Cell User Interaction #668
Changes from 37 commits
48419bd
ebbdb75
604d4e9
6d4c1dd
559e8dc
ca5e784
aff2427
45bc6a7
060cbc5
8021d27
73bd520
4db0ad7
c286076
33f37a0
a3a6c59
4b165e4
3e0bb0b
fbfdbb8
9fc2578
a66d7ca
866fb98
a8c6f62
62ea450
6d61507
7b32601
8270f29
6cd6e57
ff29832
d9cc0ee
2b837e6
40e9329
162ea87
80fe6ca
6a7bfbd
417f3e6
4012757
0a16081
3507018
d114367
9444b89
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 |
---|---|---|
|
@@ -13,7 +13,14 @@ private enum Layout { | |
} | ||
} | ||
|
||
internal protocol PledgeDescriptionCellDelegate: class { | ||
func pledgeDescriptionCellDidPresentTrustAndSafety(_ cell: PledgeDescriptionCell) | ||
} | ||
|
||
final class PledgeDescriptionCell: UITableViewCell, ValueCell { | ||
fileprivate let viewModel = PledgeDescriptionCellViewModel() | ||
internal weak var delegate: PledgeDescriptionCellDelegate? | ||
|
||
// MARK: - Properties | ||
|
||
private lazy var rootStackView: UIStackView = { UIStackView(frame: .zero) }() | ||
|
@@ -25,9 +32,11 @@ final class PledgeDescriptionCell: UITableViewCell, ValueCell { | |
private lazy var estimatedDeliveryLabel: UILabel = { UILabel(frame: .zero) }() | ||
private lazy var dateLabel: UILabel = { UILabel(frame: .zero) }() | ||
private lazy var descriptionLabel: UILabel = { UILabel(frame: .zero) }() | ||
private lazy var learnMoreLabel: UILabel = { UILabel(frame: .zero) }() | ||
private lazy var spacerView: UIView = { | ||
return UIView(frame: .zero) |> \.translatesAutoresizingMaskIntoConstraints .~ false }() | ||
private lazy var spacerView: UIView = { UIView(frame: .zero) }() | ||
private lazy var learnMoreButton: UIButton = { | ||
return MultiLineButton(type: .custom) | ||
|> \.translatesAutoresizingMaskIntoConstraints .~ false | ||
}() | ||
|
||
// MARK: - Lifecycle | ||
|
||
|
@@ -46,6 +55,8 @@ final class PledgeDescriptionCell: UITableViewCell, ValueCell { | |
|> ksr_constrainViewToEdgesInParent() | ||
|
||
self.configureStackView() | ||
self.learnMoreButton.addTarget(self, action: #selector(learnMoreButtonTapped), for: .touchUpInside) | ||
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. Can we move all this out of the
|
||
self.bindViewModel() | ||
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. Note-to-self that we're going to find a way to override |
||
|
||
NSLayoutConstraint.activate([ | ||
self.containerImageView.widthAnchor.constraint(equalToConstant: Layout.ImageView.width), | ||
|
@@ -90,17 +101,10 @@ final class PledgeDescriptionCell: UITableViewCell, ValueCell { | |
_ = self.descriptionLabel | ||
|> descriptionLabelStyle | ||
|
||
_ = self.learnMoreLabel | ||
_ = self.learnMoreButton | ||
|> checkoutBackgroundStyle | ||
_ = self.learnMoreLabel | ||
|> learnMoreLabelStyle | ||
} | ||
|
||
// MARK: - Configuration | ||
|
||
func configureWith(value: String) { | ||
_ = self.dateLabel | ||
|> \.text .~ value | ||
_ = self.learnMoreButton | ||
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. Are these styles split up for compiler reasons? Or can we do:
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. These are split because the compiler gives us an error. This is because
Let me know if I should make that change. 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. We also split it because the first part sets the background and the next the style. That way we don't have to worry about the order of passing the |
||
|> learnMoreButtonStyle | ||
} | ||
|
||
private func configureStackView() { | ||
|
@@ -112,7 +116,7 @@ final class PledgeDescriptionCell: UITableViewCell, ValueCell { | |
self.estimatedDeliveryLabel, | ||
self.dateLabel, | ||
self.descriptionLabel, | ||
self.learnMoreLabel], self.descriptionStackView) | ||
self.learnMoreButton], self.descriptionStackView) | ||
|> ksr_addArrangedSubviewsToStackView() | ||
|
||
if #available(iOS 11.0, *) { | ||
|
@@ -124,10 +128,36 @@ final class PledgeDescriptionCell: UITableViewCell, ValueCell { | |
view.heightAnchor.constraint(equalToConstant: Layout.SpacerView.height).isActive = true | ||
self.descriptionStackView.insertArrangedSubview(view, at: 3) | ||
} | ||
|
||
_ = ([self.descriptionStackView], self.rootStackView) | ||
|> ksr_addArrangedSubviewsToStackView() | ||
} | ||
|
||
// MARK: - Binding | ||
|
||
internal override func bindViewModel() { | ||
super.bindViewModel() | ||
|
||
self.dateLabel.rac.text = self.viewModel.outputs.estimatedDeliveryText | ||
|
||
self.viewModel.outputs.presentTrustAndSafety | ||
.observeForUI() | ||
.observeValues { [weak self] in | ||
guard let _self = self else { return } | ||
self?.delegate?.pledgeDescriptionCellDidPresentTrustAndSafety(_self) | ||
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. Man, this seems really confusing (using two
|
||
} | ||
} | ||
|
||
// MARK: - Actions | ||
|
||
@objc private func learnMoreButtonTapped() { | ||
self.viewModel.inputs.tapped() | ||
} | ||
|
||
// MARK: - Configuration | ||
|
||
internal func configureWith(value: String) { | ||
self.viewModel.inputs.configureWith(estimatedDeliveryDate: value) | ||
} | ||
} | ||
|
||
private let rootStackViewStyle: StackViewStyle = { (stackView: UIStackView) in | ||
|
@@ -172,11 +202,14 @@ private let descriptionLabelStyle: LabelStyle = { (label: UILabel) in | |
|> \.numberOfLines .~ 0 | ||
} | ||
|
||
private let learnMoreLabelStyle: LabelStyle = { (label: UILabel) in | ||
label | ||
|> \.text %~ { _ in "\(Strings.Learn_more_about_accountability())." } | ||
|> \.textColor .~ UIColor.ksr_green_500 | ||
|> \.font .~ UIFont.ksr_caption1() | ||
|> \.adjustsFontForContentSizeCategory .~ true | ||
|> \.numberOfLines .~ 0 | ||
private let learnMoreButtonStyle = { (button: UIButton) -> UIButton in | ||
button | ||
|> UIButton.lens.titleColor(for: .normal) .~ UIColor.ksr_green_500 | ||
|> UIButton.lens.titleLabel.font %~~ { _, label in | ||
label.traitCollection.isRegularRegular ? .ksr_body(size: 17.0) : .ksr_body(size: 14.0) | ||
} | ||
|> UIButton.lens.contentHorizontalAlignment .~ .left | ||
|> UIButton.lens.titleColor(for: .highlighted) .~ .ksr_text_dark_grey_500 | ||
|> UIButton.lens.title(for: .normal) %~ { _ in | ||
return Strings.Learn_more_about_accountability() } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -230,7 +230,8 @@ internal final class DeprecatedRewardPledgeViewController: UIViewController { | |
|
||
_ = self.disclaimerButton | ||
|> UIButton.lens.accessibilityLabel %~ { _ in | ||
Strings.Kickstarter_is_not_a_store_Its_a_way_to_bring_creative_projects_to_life() | ||
Strings.Kickstarter_is_not_a_store() | ||
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. Thanks for reverting this back..missed it in my prior PR review |
||
+ " " + Strings.Its_a_way_to_bring_creative_projects_to_life() | ||
+ " " + Strings.Learn_more_about_accountability() | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,22 @@ class PledgeTableViewController: UITableViewController { | |
let footerView = tableView.dequeueReusableHeaderFooterView(withClass: PledgeFooterView.self) | ||
return footerView | ||
} | ||
|
||
internal override func tableView(_ tableView: UITableView, | ||
willDisplay cell: UITableViewCell, | ||
forRowAt indexPath: IndexPath) { | ||
if let descriptionCell = cell as? PledgeDescriptionCell { | ||
descriptionCell.delegate = self | ||
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. Could have used prelude for this |
||
} | ||
} | ||
} | ||
|
||
extension PledgeTableViewController: PledgeDescriptionCellDelegate { | ||
internal func pledgeDescriptionCellDidPresentTrustAndSafety(_ cell: PledgeDescriptionCell) { | ||
let vc = HelpWebViewController.configuredWith(helpType: .trust) | ||
let nav = UINavigationController(rootViewController: vc) | ||
self.present(nav, animated: true, completion: nil) | ||
} | ||
} | ||
|
||
// MARK: - Styles | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import Foundation | ||
import KsApi | ||
import Prelude | ||
import ReactiveSwift | ||
import ReactiveExtensions | ||
import Result | ||
|
||
public protocol PledgeDescriptionCellViewModelInputs { | ||
func configureWith(estimatedDeliveryDate: String) | ||
func tapped() | ||
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. Can we use a more specific name for this input? Maybe |
||
} | ||
|
||
public protocol PledgeDescriptionCellViewModelOutputs { | ||
var estimatedDeliveryText: Signal<String, NoError> { get } | ||
var presentTrustAndSafety: Signal<Void, NoError> { get } | ||
} | ||
|
||
public protocol PledgeDescriptionCellViewModelType { | ||
var inputs: PledgeDescriptionCellViewModelInputs { get } | ||
var outputs: PledgeDescriptionCellViewModelOutputs { get } | ||
} | ||
|
||
public final class PledgeDescriptionCellViewModel: PledgeDescriptionCellViewModelType, | ||
PledgeDescriptionCellViewModelInputs, PledgeDescriptionCellViewModelOutputs { | ||
|
||
public init() { | ||
self.estimatedDeliveryText = self.estimatedDeliveryDateProperty.signal | ||
|
||
self.presentTrustAndSafety = self.tappedProperty.signal | ||
} | ||
|
||
private let estimatedDeliveryDateProperty = MutableProperty<String>("") | ||
public func configureWith(estimatedDeliveryDate: String) { | ||
self.estimatedDeliveryDateProperty.value = estimatedDeliveryDate | ||
} | ||
private let tappedProperty = MutableProperty(()) | ||
public func tapped() { | ||
self.tappedProperty.value = () | ||
} | ||
|
||
public let estimatedDeliveryText: Signal<String, NoError> | ||
public let presentTrustAndSafety: Signal<Void, NoError> | ||
|
||
public var inputs: PledgeDescriptionCellViewModelInputs { return self } | ||
public var outputs: PledgeDescriptionCellViewModelOutputs { return self } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import XCTest | ||
@testable import KsApi | ||
@testable import Library | ||
@testable import ReactiveExtensions_TestHelpers | ||
import Prelude | ||
import Result | ||
|
||
internal final class PledgeDescriptionCellViewModelTests: TestCase { | ||
private let vm: PledgeDescriptionCellViewModelType = PledgeDescriptionCellViewModel() | ||
|
||
private let estimatedDeliveryText = TestObserver<String, NoError>() | ||
private let presentTrustAndSafety = TestObserver<Void, NoError>() | ||
|
||
override func setUp() { | ||
super.setUp() | ||
|
||
self.vm.outputs.estimatedDeliveryText.observe(estimatedDeliveryText.observer) | ||
self.vm.outputs.presentTrustAndSafety.observe(presentTrustAndSafety.observer) | ||
} | ||
|
||
func testEstimatedDeliveryDate() { | ||
let estimatedDelivery = 1468527587.32843 | ||
|
||
let date = Format.date(secondsInUTC: estimatedDelivery, template: "MMMMyyyy", timeZone: UTCTimeZone) | ||
|
||
self.vm.inputs.configureWith(estimatedDeliveryDate: date) | ||
self.estimatedDeliveryText.assertValues(["July 2016"], "Emits the estimated delivery date") | ||
} | ||
|
||
func testPresentTrustAndSafety() { | ||
self.vm.inputs.tapped() | ||
self.presentTrustAndSafety.assertDidEmitValue() | ||
} | ||
} |
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.
Do we need the closure here? Or can't we just do
private lazy var spacerView = UIView(frame: .zero)
?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.
Good point! We actually don't need the closure. So there's going to be a number of places that we can change this. I think the function stuff is a hangover from having set local vars in that scope before to set properties on it, but seeing as we pipe forward to set these things it can all be done in-line.