Zipcode field in Add New Card screen #566

ifbarrera · 2019-02-01T16:27:36Z

📲 What

Adds a required zipcode/postal code field to the "Add new card" flow.

🛠 How

created a new generic SettingsFormFieldView which contains a label and a text field. This can be used on storyboards and configured as needed, but will have the same consistent styling
added a SettingsFormFieldView instance to the add new card stackView, configured titleLabel text

Additional work:

scroll view now increases the contentInset when the keyboard displays
form now submits and keyboard dismisses when you hit "Save" and when you hit "Return" on the last field (zipcode field)
updated form constraints to layoutMargins and set margins on the stack view

Known bugs:

(iOS 10): zipcode is incorrectly focused after completing the credit card number field. This seems to be a bug with how the Stripe view is transitioning from one internal text field to another before calling paymentCardFieldDidFinishEditing()

Tested on:

Sade (iOS 10.3.1)
- Portrait mode, Landscape mode
Elvis (iOS 11.2.2)
- Portrait mode, Landscape mode
iPhone X (iOS 12.1.2)
- Portrait, Landscape
Simulator iPhone 5 iOS 10.3.1
Simulator iPad Pro (11-inch) iOS 12.1

👀 See

♿️ Accessibility

Tap targets use minimum of 44x44 pts dimensions
Works with VoiceOver
Supports Dynamic Type

✅ Acceptance criteria

Navigate to "Add card screen", enter name, credit card details, zipcode, user should be able to submit form
"Save" button should not be enabled if no zipcode is entered

…ter/ios-oss into ksr-support-credit-cards

…ter/ios-oss into new-card-zipcode # Conflicts: # Kickstarter-iOS/Assets.xcassets/icons/icon--plus.imageset/Contents.json # Kickstarter-iOS/Locales/Base.lproj/Localizable.strings # Kickstarter-iOS/Locales/de.lproj/Localizable.strings # Kickstarter-iOS/Locales/es.lproj/Localizable.strings # Kickstarter-iOS/Locales/fr.lproj/Localizable.strings # Kickstarter-iOS/Locales/ja.lproj/Localizable.strings # Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift # Kickstarter-iOS/Views/Storyboards/Settings.storyboard # Library/Strings.swift # Library/ViewModels/AddNewCardViewModel.swift

ifbarrera · 2019-02-01T16:32:41Z

Kickstarter-iOS/Views/Storyboards/LoadingBarButtonItemView.xib

@@ -20,7 +20,7 @@
                    <rect key="frame" x="0.0" y="0.0" width="375" height="667"/>


Repositioned the activity indicator to aligned to the trailing edge of the UIButton, and right aligned the titleLabel within the UIButton

ifbarrera · 2019-02-01T16:34:07Z

Library/ViewModels/AddNewCardViewModelTests.swift


  override func setUp() {
    super.setUp()
-    self.vm.outputs.activityIndicatorShouldShow.observe(activityIndicatorShouldShow.observer)


Added self to all the test observer references.

ifbarrera · 2019-02-01T16:38:25Z

Kickstarter-iOS/Views/Storyboards/Settings.storyboard

@@ -738,7 +738,7 @@
                                <rect key="frame" x="0.0" y="20" width="375" height="647"/>
                                <subviews>
                                    <stackView opaque="NO" contentMode="scaleToFill" axis="vertical" translatesAutoresizingMaskIntoConstraints="NO" id="lnb-P0-tav">
-                                        <rect key="frame" x="0.0" y="0.0" width="375" height="176"/>


Added the SettingsFormFieldView to the main stack view

changed all left & right constraints within stack view elements to be relative to the layoutMargins, then set the layoutMargins of the stack view

updated the height of all the separators to be 0.5pts (checked this value with Danny)

ifbarrera · 2019-02-01T16:43:58Z

Zip code for iOS payment methods

justinswart

Some minor comments, going to run this on device and test 👍

justinswart · 2019-02-04T22:26:38Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

@@ -196,6 +226,15 @@ STPPaymentCardTextFieldDelegate, MessageBannerViewControllerPresenting {
    self.viewModel.inputs.cardholderNameTextFieldReturn()


The parenthesis on the line just above this seems to have shifted weirdly.

justinswart · 2019-02-04T22:28:04Z

Kickstarter-iOS/Views/SettingsFormFieldView.swift

+    super.init(coder: aDecoder)
+
+    guard let view = self.view(fromNib: .SettingsFormFieldView) else {
+        fatalError("Failed to load view")


swiftlint missed this 4-space indent 🤔

justinswart · 2019-02-04T22:33:15Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

  // MARK: - Private Functions
-  private func createStripeToken(cardholderName: String, cardNumber: String, expirationMonth: Month,
-                                 expirationYear: Year, cvc: String) {
+  private func createStripeToken(paymentDetails: PaymentDetails) {


can I suggest:

private func createStripeToken(with paymentDetails: PaymentDetails) { ... }

So that the callsite reads:

self?.createStripeToken(with: paymentDetails)

justinswart · 2019-02-04T22:34:53Z

Library/ViewModels/AddNewCardViewModelTests.swift

+    self.saveButtonIsEnabled.assertDidNotEmitValue()
+
+    self.vm.inputs.cardholderNameChanged("Native Squad")
+    self.vm.inputs.creditCardChanged(cardDetails: ("4242 4242 4242 4242", 11, 99, "123"))


Do we need to test a scenario where the card has expired?

justinswart · 2019-02-04T22:48:57Z

Could we force capitals in the zip code field? This is useful when typing a Canadian postal code like V6B 1M1

Scollaco

Well done, @ifbarrera! I just left a few suggestions.

Scollaco · 2019-02-04T20:56:17Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

@@ -196,6 +226,15 @@ STPPaymentCardTextFieldDelegate, MessageBannerViewControllerPresenting {
    self.viewModel.inputs.cardholderNameTextFieldReturn()
  }

+  // MARK: - Zipcode UITextField Delegate


For consistency, could we move the delegate functions to an extension?

Scollaco · 2019-02-04T22:07:47Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

+  @objc internal func zipcodeTextFieldChanged(textField: UITextField) {
+    self.viewModel.inputs.zipcodeChanged(zipcode: textField.text)
+  }
+
  // MARK: - STPPaymentCardTextFieldDelegate


Same here (even though it's already merged, it would be good to keep consistency)

Scollaco · 2019-02-04T22:26:01Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

@@ -58,6 +62,12 @@ STPPaymentCardTextFieldDelegate, MessageBannerViewControllerPresenting {
    let navigationBarButton = UIBarButtonItem(customView: self.saveButtonView)
    self.navigationItem.setRightBarButton(navigationBarButton, animated: false)

+    self.zipcodeView.textField.addTarget(self, action: #selector(zipcodeTextFieldDoneEditing),


I think here it would be a good idea to move the textfield actions to the SettingsFormFieldView and create a SettingsFormFieldViewDelegate that sends a message whenever the functions are called. This way we don't need to access the IBOutlets, and keep them private. In this case, I wiuld rename the outlet to zipcodeTextField, since even though its a UIView, it represents a textfield.

Just curious, what do you think is the benefit of keeping the outlets private? This view element is intended to be customized as-needed by the owning view (in this case the AddNewCardViewController). Adding a delegate to transmit the UITextFieldDelegate messages seems like unnecessary boilerplate that doesn't actually provide any additional functionality. Just trying to understand what the motivation for making the outlets private is in this case.

I was just thinking that would be a better practice to avoid accessing IBOutlets from outside the class, and treat SettingsFormFieldView as a TextField representation, instead of an UIView container. Since the zipcodeTextField represents a textField, this approach would let the class implement the UITextFieldDelegate messages itself, keeping it in its own context. The tradeoff is the implementation of the SettingsFormFieldViewDelegate, in other hand, though, we could get rid of the addTarget methods. But this was just a suggestion and a different idea to solve the same problem.

I hear you, and if this view is going to be reused in a bunch of places it would be nice to get rid of the addTarget methods. I'd say if we end up using this view again we can re-think where theUITextFieldDelegate methods should live.

justinswart

lgtm!

* New card screen ui (#471) * add card view controller * transition to add new card and uibarbuttons * settings style applied to add new card screen * changed keyboard style for textfields * view controller tests * placeholder for cardholder name textfield * Record new screenshots * new strings, added strings as place holder and text label * pr feedback - inferring type for cancel button tintcolor * pr feedback - keypath, no more lenses * swiftlint fix * new snapshots * Payment methods (#457) * Stripe element add new card (#473) * New card screen ui (#471) * add card view controller * transition to add new card and uibarbuttons * settings style applied to add new card screen * changed keyboard style for textfields * view controller tests * placeholder for cardholder name textfield * Record new screenshots * new strings, added strings as place holder and text label * pr feedback - inferring type for cancel button tintcolor * pr feedback - keypath, no more lenses * swiftlint fix * new snapshots * new stppaymentcardtextfield showing * wip - placeholder colors * snapshot tests * Email undeliverable/unverified (#478) * Adding emailIsVerified * Renaming GraphUserEmail * ViewModel tests * ChangeEmailViewController tests * Strings, swiftlint * Strings & new screenshots * Cleanup * PR comments * PR updates * Test naming * pr feedback * new snapshots * new placeholder colors * new snapshots * Delete payment methods (#479) * Add payment method deletion * Test datasource * Test view model * Clean-up and address PR feedback * Rename credit card -> payment method * Fix test * Add credit card implementation (#503) * Email undeliverable/unverified (#478) * Adding emailIsVerified * Renaming GraphUserEmail * ViewModel tests * ChangeEmailViewController tests * Strings, swiftlint * Strings & new screenshots * Cleanup * PR comments * PR updates * Test naming * Use correct function to log events in crash logs (#481) * Use correct function to log events in crash logs * Fix indentation * changed ui colors for textfield text and font size of text label * vm and work on enabling save * wip- get stripe token * wip- payment source mutation * wip- keyboard response * wip- getting a stripe error here * wip- error fix w/ publishable key, error banner showing * wip- saving with error and keyboard functionality * wip- updating card immediately * wip * wip - ACs met * wip - refactor in view model, made IDs testable, begane VM tests * wip -refactor on vm/vmtest * wip -refactor deleted comments on vmt * wip -snapshot tests * swiftlint fixes * renaming/refactor, corrected paymentmethodstests * pr feedback * swiftlint fixes * changed function name * indentation * Payment methods event tracking (#496) * Reverted code that deleted SettingsNewsletters from Storyboard.swift * swiftlint * Reverted code to instantiate settings newsletters vc on tests * Alphabetized storyboard enum * Settings payments colors (#530) * Unsupported Credit Cards (#561) * Viewmodel logic for unsupported cards * Tests and accessibility fixes * Simplify add card button * Screenshots and string updates * Cleanup design and remove logs * Improving comment message * Removing filter debug builds * Addressing PR comments * Updating payment methods screenshots * Screenshots * Possible fix for alignment issues * Moving iOS 10 handling to output signal closure * Remove line * Renaming * Last cleanup * Strings & Asset update * Switching from unsupported to supported * Zipcode field in Add New Card screen (#566) * Viewmodel logic for unsupported cards * Tests and accessibility fixes * Simplify add card button * Screenshots and string updates * Cleanup design and remove logs * Improving comment message * Removing filter debug builds * Addressing PR comments * Updating payment methods screenshots * Screenshots * Possible fix for alignment issues * Moving iOS 10 handling to output signal closure * Remove line * Renaming * Last cleanup * Shared styled form field * Adding functionality for zipcode form field * More functionality * View model & screenshot tests * Swiftlint * Cleanup * Improving test and cleanup * PR feedback and autocapitalizing zipcode * Regenerating ChangePassword screenshots * [Payment methods] CVC bug fix (#574) * wip * wip * wip * wip-swiftlint * moved CreatePaymentSourceEnvelope to its own file, Mockservice * swiftlint fix * deleted unnecesary debugging code * new paymentsource temploate and vm tests * swiftlint fix * fixed vm test * PR feedback * update schema * [Payment methods] Minor bug fixes (#579) * tableViewIsEditing false and show banner after dismiss * Swiftlint * Adding StripePublishableKey to example file * Update publishableKey test * Spacing * Point-free helper * [Payment Methods] Update padding and image view size (#581) * Update padding and image view size * Update stack view's spacing * Update snapshots * Use layoutMargins to set the padding * [Payment methods] Disable edit button if no payment methods (#586) * [Payment methods] Bugs & visual fixes (#578) * Adding custom UITableViewHeader and always calling reload data on viewDidLoad * Updating screenshots * Cleanup autolayout warnings * Screenshots * PR comments * No autolayout for footer view * Cleanup * Remove intrinsicContentSize * Tableview appears without delay fix (#587) * Fix header/footer auto sizing * Update header background color * Update snapshots * Screenshots again * Updating constraint priority to resolve ambiguity * [Payment methods] Refresh payment methods table view properly (#588) * Refresh payment methods table view properly * Remove reference to weak self * Rename delegate methods * [Payment methods] Refetch payment methods on viewDidLoad or explicitly (by delegate) (#591) * Do not refresh on viewWillAppear * Use optional string to avoid unnecessary initializer * Bring back viewWillAppear signal to better reflect view controller's lifecycle state * [Payment methods] A11y - Credit card name (#593) * Add card name to the a11y label * Add comment * [Payment Methods] Design Fixes (#592) * Design fixes * Fresh screenshots ✨ * Remove recordMode * Deleting stale screenshots * Re-add accidentally deleted line * update strings * [Payment Methods] Support optional card type and error handling for payment methods (#596) * Optional card type, payment methods load error * Optional support for imageName * Fix accessibility label * Fix merge conflict spacing * Swiftlint * Implementing PR suggestions * Fixed bug that shows wrong expiration date (#595) * [Payment Methods] Add New Card Design Fixes (#594) * error message present and return key to done * removed error message banner for incomplete payment details * new snapshots for correct insets * [Payment methods] Optimistically disable edit button on card deletion (#589) * Disable edit button optimistically, use stored cards in result from card deletion * Fix issue where edit button is not re-enabled upon the failed deletion of the last card. * Remove unnecessary .init * Add skipRepeats() to editButtonIsEnabled signal. * Fixing potential reference cycles (#598)

ifbarrera and others added 22 commits January 18, 2019 17:05

Viewmodel logic for unsupported cards

052b133

Tests and accessibility fixes

aef03d2

Simplify add card button

bf4e82f

Screenshots and string updates

7c90fd2

Cleanup design and remove logs

e05c0dc

Improving comment message

bbbed98

Merge branch 'feature-payment-methods' of https://github.com/kickstar…

cca98dc

…ter/ios-oss into ksr-support-credit-cards

Removing filter debug builds

0506dc7

Addressing PR comments

fe77ce9

Updating payment methods screenshots

336986c

Screenshots

bf9b41f

Possible fix for alignment issues

7f775a2

Moving iOS 10 handling to output signal closure

66c0044

Remove line

1943466

Renaming

8de7eb6

Last cleanup

69da6cb

Shared styled form field

8e30978

Adding functionality for zipcode form field

251f60a

More functionality

8f58093

View model & screenshot tests

56a2ad3

Swiftlint

dd31519

ifbarrera commented Feb 1, 2019

View reviewed changes

Cleanup

56c9c26

ifbarrera added the needs review label Feb 1, 2019

ifbarrera assigned justinswart Feb 1, 2019

ifbarrera requested review from Scollaco, cdolm92 and dusi February 1, 2019 16:35

ifbarrera commented Feb 1, 2019

View reviewed changes

justinswart reviewed Feb 4, 2019

View reviewed changes

Improving test and cleanup

e655e8b

Scollaco reviewed Feb 5, 2019

View reviewed changes

PR feedback and autocapitalizing zipcode

de4bcde

justinswart approved these changes Feb 6, 2019

View reviewed changes

ifbarrera merged commit 22785ea into feature-payment-methods Feb 6, 2019

ifbarrera deleted the new-card-zipcode branch February 6, 2019 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zipcode field in Add New Card screen #566

Zipcode field in Add New Card screen #566

ifbarrera commented Feb 1, 2019 •

edited by justinswart

Loading

ifbarrera Feb 1, 2019

ifbarrera Feb 1, 2019 •

edited

Loading

ifbarrera Feb 1, 2019

ifbarrera commented Feb 1, 2019

justinswart left a comment

justinswart Feb 4, 2019

justinswart Feb 4, 2019

justinswart Feb 4, 2019

justinswart Feb 4, 2019

justinswart commented Feb 4, 2019

Scollaco left a comment

Scollaco Feb 4, 2019

Scollaco Feb 4, 2019

Scollaco Feb 4, 2019

ifbarrera Feb 5, 2019

Scollaco Feb 6, 2019

ifbarrera Feb 6, 2019

justinswart left a comment

		@@ -20,7 +20,7 @@
		<rect key="frame" x="0.0" y="0.0" width="375" height="667"/>

		@@ -196,6 +226,15 @@ STPPaymentCardTextFieldDelegate, MessageBannerViewControllerPresenting {
		self.viewModel.inputs.cardholderNameTextFieldReturn()

Zipcode field in Add New Card screen #566

Zipcode field in Add New Card screen #566

Conversation

ifbarrera commented Feb 1, 2019 • edited by justinswart Loading

📲 What

🛠 How

👀 See

♿️ Accessibility

✅ Acceptance criteria

Choose a reason for hiding this comment

ifbarrera Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifbarrera commented Feb 1, 2019

justinswart left a 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

Choose a reason for hiding this comment

justinswart commented Feb 4, 2019

Scollaco left a 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

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

ifbarrera commented Feb 1, 2019 •

edited by justinswart

Loading

ifbarrera Feb 1, 2019 •

edited

Loading