[NT-312] Pledge amount invalid input color #849

dusi · 2019-09-21T06:20:13Z

📲 What

When the input value is lower than the minimum or higher than the maximum the text turns red
When user dismisses keyboard by tapping anywhere on the screen (and the input is still invalid) it stays this way (instead of clamping it to either min or max)

🤔 Why

Some UX improvements to amount input

🛠 How

See inline comments

👀 See

Before 🐛	After 🦋

✅ Acceptance criteria

TextField input

Entering value that is smaller than minimum pledge value should make the text red
Entering value that is larger than maximum pledge value should make the text red
Entering 0 disables the decrement button of the stepper
Entering Double max disables the increment button of the stepper (this will be harder to test so it might be sufficient to review snapshots and integration tests)

Stepper input

Entering value that is smaller than minimum pledge value should make the text red
Entering value that is larger than maximum pledge value should make the text red
Entering 0 disables the decrement button of the stepper
Entering Double max disables the increment button of the stepper (this will be harder to test so it might be sufficient to review snapshots and integration tests)

It's possible that the last criteria will be better tested when we remove decimal support in some of the upcoming work.

⏰ TODO

Update Cartfile when Add textColor property extension to UITextField Kickstarter-ReactiveExtensions#79 gets merged
Accessibility will be handled separately

dusi · 2019-09-21T06:26:48Z

Kickstarter-iOS/TestHelpers/TraitCollection.swift

+import UIKit
+
+extension UITraitCollection {
+  static let allCases: [UITraitCollection] = [


Just a test helper for allowing us testing large text better.

dusi · 2019-09-21T06:26:59Z

Kickstarter-iOS/Views/AmountInputView.swift

@@ -112,15 +112,13 @@ private let labelStyle: LabelStyle = { (label: UILabel) in
    |> \.font .~ UIFont.ksr_body()
    |> \.adjustsFontForContentSizeCategory .~ true
    |> \.textAlignment .~ NSTextAlignment.right
-    |> \.textColor .~ UIColor.ksr_green_500


These are now set using VM's outputs

dusi · 2019-09-21T06:27:37Z

Kickstarter-iOS/Views/Controllers/PledgeAmountViewController.swift

-      |> \.translatesAutoresizingMaskIntoConstraints .~ false
-  }()
-
+  private lazy var horizontalSpacer: UIView = { UIView(frame: .zero) }()


No real need for auto layout anymore...we're using spacing (where we have previously used auto layout)...this should be simpler and more readable.

dusi · 2019-09-21T06:28:02Z

Kickstarter-iOS/Views/Controllers/PledgeAmountViewController.swift

@@ -40,16 +36,14 @@ final class PledgeAmountViewController: UIViewController {
      |> ksr_addSubviewToParent()
      |> ksr_constrainViewToEdgesInParent()

-    _ = ([self.titleLabel, self.adaptableStackView], self.rootStackView)
+    _ = ([self.titleLabel, self.adaptableStackView, UIView(frame: .zero)], self.rootStackView)


This makes sure that the vertical stack view will always align its subviews vertically to the top

dusi · 2019-09-21T06:28:52Z

Kickstarter-iOS/Views/Controllers/PledgeAmountViewController.swift

      |> ksr_addArrangedSubviewsToStackView()

    self.amountInputView.textField.delegate = self

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


We have previously used this width as the minimum space between the stepper and the input field (otherwise when the text field grows it would almost touch the stepper)...now this is done using spacing on the stack view which should be much simpler and easier to reason about.

dusi · 2019-09-21T06:29:49Z

Kickstarter-iOS/Views/Controllers/PledgeAmountViewControllerTests.swift

+import Prelude
+import UIKit
+
+final class PledgeAmountViewControllerTests: TestCase {


I felt like it was a perfect opportunity to add testing...not only can we test large text but also invalid input values, currency symbol, stepper increment/decrement button etc.

dusi · 2019-09-21T06:30:15Z

Kickstarter-iOS/Views/Controllers/PledgeViewControllerTests.swift

+    }
+  }
+
+  func testView_WithShippingLocation() {


TODO: Rename to something more meaningful

dusi · 2019-09-21T06:31:45Z

Library/ViewModels/PledgeAmountViewModel.swift

-    self.stepperMaxValue = minAndMax.signal
-      .map(second)
+    self.stepperMinValue = minValue.mapConst(0)
+    self.stepperMaxValue = minValue.mapConst(Double.greatestFiniteMagnitude)


I've made a decision to use 0...Double.max but we can chose something else as well (I'm open to discussing this).

The reason we're able to do 0...Double.max is because we want to allow the user to actually decrement/increment bellow/above pledge's min/max value...in that case we simply change the input text to red to indicate this out of bounds state.

ksr-ci-bot · 2019-09-21T06:46:50Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

dusi · 2019-09-21T07:02:59Z

Cartfile.resolved

@@ -4,7 +4,7 @@ github "Alamofire/AlamofireImage" "c41f8b0acfbb3180fe045df73596e4332c338633"
 github "ReactiveCocoa/ReactiveSwift" "6.0.0"
 github "facebook/facebook-objc-sdk" "v5.0.2"
 github "kickstarter/Kickstarter-Prelude" "d03f8831dcec9ad157db4f1c5f769ec17e1a3766"
-github "kickstarter/Kickstarter-ReactiveExtensions" "aa994f0921dca1965ae537fe77c2edc798eef230"
+github "kickstarter/Kickstarter-ReactiveExtensions" "99c3013e22fd6d4ea29d9d4c7a4a7b10bf80a99c"


It's weird that it builds on Circle and not on my machine. I wonder what's the real problem here. 🤔

The hash in this file should match the one in the Cartfile.

dusi · 2019-09-21T07:08:12Z

Library/ViewModels/PledgeAmountViewModel.swift

    )

    self.textFieldIsFirstResponder = self.doneButtonTappedProperty.signal
      .mapConst(false)
+
+    self.textFieldTextColor = textColor
+      .wrapInOptional()


Unfortunately it seemed necessary to pass textColor as an optional to a UITextField (which is how it's also defined in UIKit)

justinswart

All seems good! Just the Carthage.resolved comment and then a question. The abstract seems to require that the Total is also red?

justinswart · 2019-09-24T17:19:44Z

Cartfile.resolved

@@ -4,7 +4,7 @@ github "Alamofire/AlamofireImage" "c41f8b0acfbb3180fe045df73596e4332c338633"
 github "ReactiveCocoa/ReactiveSwift" "6.0.0"
 github "facebook/facebook-objc-sdk" "v5.0.2"
 github "kickstarter/Kickstarter-Prelude" "d03f8831dcec9ad157db4f1c5f769ec17e1a3766"
-github "kickstarter/Kickstarter-ReactiveExtensions" "aa994f0921dca1965ae537fe77c2edc798eef230"
+github "kickstarter/Kickstarter-ReactiveExtensions" "99c3013e22fd6d4ea29d9d4c7a4a7b10bf80a99c"


The hash in this file should match the one in the Cartfile.

justinswart

legit!

dusi added 8 commits September 20, 2019 14:53

Update reactive extensions dependency

5f5363d

Set text color based on min / max

35e92d6

Add pledge vc snapshot tests

ca45fd5

Rename signals

c083ccc

Add trait collection test helper

4b0b48b

Align pledge amount stack view to the top (using a vertical spacer)

f656dd0

Remove logging

b0df1b3

Add pledge amount snapshot tests

5a0f585

dusi assigned justinswart Sep 21, 2019

dusi mentioned this pull request Sep 21, 2019

Add textColor property extension to UITextField kickstarter/Kickstarter-ReactiveExtensions#79

Merged

dusi commented Sep 21, 2019

View reviewed changes

Remove snapshots

509dfeb

Format code

0538c2b

dusi commented Sep 21, 2019

View reviewed changes

dusi added 2 commits September 21, 2019 00:49

Rename snapshot test

8364de9

Update reactive extensions dependency

116fb02

dusi added the needs review label Sep 23, 2019

dusi requested a review from ifbarrera September 23, 2019 19:39

justinswart suggested changes Sep 24, 2019

View reviewed changes

Update Cartfile.resolved

7d5e9b2

dusi requested a review from justinswart September 25, 2019 23:39

Merge branch 'master' into pledge-amount-input-invalid

841e916

justinswart approved these changes Sep 26, 2019

View reviewed changes

dusi merged commit ed41a9a into master Sep 26, 2019

dusi deleted the pledge-amount-input-invalid branch October 4, 2019 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-312] Pledge amount invalid input color #849

[NT-312] Pledge amount invalid input color #849

dusi commented Sep 21, 2019 •

edited

Loading

dusi Sep 21, 2019

dusi Sep 21, 2019

dusi Sep 21, 2019

dusi Sep 21, 2019 •

edited

Loading

dusi Sep 21, 2019

dusi Sep 21, 2019

dusi Sep 21, 2019

dusi Sep 21, 2019

ksr-ci-bot commented Sep 21, 2019 •

edited

Loading

dusi Sep 21, 2019

justinswart Sep 24, 2019

dusi Sep 21, 2019

justinswart left a comment

justinswart Sep 24, 2019

justinswart left a comment

[NT-312] Pledge amount invalid input color #849

[NT-312] Pledge amount invalid input color #849

Conversation

dusi commented Sep 21, 2019 • edited Loading

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

⏰ TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusi Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksr-ci-bot commented Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart left a comment

Choose a reason for hiding this comment

dusi commented Sep 21, 2019 •

edited

Loading

dusi Sep 21, 2019 •

edited

Loading

ksr-ci-bot commented Sep 21, 2019 •

edited

Loading