[ 💳 Native Checkout ] Refresh Card List #835

cdolm92 · 2019-09-11T21:37:27Z

📲 What

User can now add new credit card from the pledge screen.

🤔 Why

Users can now add a preferred credit card without leaving the pledge flow.

🛠 How

We have a method called insertNewCard(_ newCard: GraphUserCreditCard.CreditCard) that inserts the card in the first position of the cardStackView. We also made changes with how the card is configured. In the cards configureWith() we now have the parameter isNew: Bool so that it is in the selected state when added.

👀 See

✅ Acceptance criteria

Note: Test this on staging. Make sure the recommended projects setting is disabled and use a test card from here: https://stripe.com/docs/testing

Navigate to pledge screen and tap on add new card
Enter test card details and tap Done when done.
You should now see the newly added card in a selected state

Further Testing:

You will need to add at least two test cards. Tap Select on more than one card. Only last card tapped should remain selected.
Deselect/Select a card

…card-list-refresh

ksr-ci-bot · 2019-09-17T22:04:44Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

…card-list-refresh

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

justinswart

Hey I have some suggestions, let me know if you want to pair.

justinswart · 2019-09-20T20:39:20Z

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

@@ -67,6 +74,9 @@ final class PledgeCreditCardView: UIView {
    _ = self.selectButton
      |> cardSelectButtonStyle


I think we should have cardSelectButtonStyle as the default style and emit a new style from the VM based on whether or not the card is selected, similar to how we've done it in RewardCardContainerViewModel. Let's also add a cardSelectedButtonStyle and use the mixDarker/mixLighter to get the highlighted/disabled backgroundColors.

This selected color looks like it's disabled though:

We should also emit the button title from the VM so that we can test it.

justinswart · 2019-09-20T20:39:50Z

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

+    self.viewModel.inputs.configureWith(creditCard: value, isNew: isNew)
+  }
+
+  public func updateButtonState(_ selected: Bool) {


We probably won't need this function if we're emitting the button style and title from the VM.

justinswart · 2019-09-20T20:45:33Z

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

+  func didSelectCard(_ cardView: PledgeCreditCardView)
+}
+
+public class PledgeCreditCardView: UIView {
  // MARK: - Properties

  private let viewModel: CreditCardCellViewModelType = CreditCardCellViewModel()


Might be a good time to rename this to PledgeCreditCardViewModel.

justinswart · 2019-09-20T20:47:31Z

Library/ViewModels/CreditCardCellViewModel.swift

+  var newlyAddedCardSelected: Signal<Bool, Never> { get }
+
+  /// Emits that select button should be in the selected state.
+  var notifyButtonTapped: Signal<Void, Never> { get }


This can be notifyDelegateButtonTapped just for clarity. Maybe even notifyDelegateSelectButtonTapped? 🤔

justinswart · 2019-09-20T20:56:37Z

Library/ViewModels/CreditCardCellViewModel.swift

  }

  fileprivate let cardProperty = MutableProperty<GraphUserCreditCard.CreditCard?>(nil)
-  public func configureWith(creditCard: GraphUserCreditCard.CreditCard) {
+  fileprivate let isNewCardProperty = MutableProperty(false)
+  public func configureWith(creditCard: GraphUserCreditCard.CreditCard, isNew: Bool) {


I think for something like this we typically have one propery like MutableProperty<(GraphUserCreditCard.CreditCard, Bool)> and maybe call it configDataPropery and then in the init we split it out. Have we used this pattern of two properties elsewhere?

Going to change that but yes, we have used the two property pattern before:

https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/SettingsNewslettersCellViewModel.swift#L104
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/SettingsNotificationCellViewModel.swift#L176
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/TwoFactorViewModel.swift#L166
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/DashboardProjectsDrawerCellViewModel.swift#L61
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/DashboardTitleViewViewModel.swift#L89
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/DiscoveryPostcardViewModel.swift#L268
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/FindFriendsFriendFollowCellViewModel.swift#L178
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/FindFriendsStatsCellViewModel.swift#L65
https://github.com/kickstarter/ios-oss/blob/master/Library/ViewModels/LoginViewModel.swift#L210

Oh ok, well I wouldn't worry about it too much then.

justinswart · 2019-09-20T21:00:57Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

@@ -11,6 +11,10 @@ internal protocol AddNewCardViewControllerDelegate: AnyObject {
    didSucceedWithMessage message: String
  )
  func addNewCardViewControllerDismissed(_ viewController: AddNewCardViewController)
+  func addNewCardViewController(


This can be:

func addNewCardViewController( _ viewController: AddNewCardViewController, didAdd newCard: GraphUserCreditCard.CreditCard )

justinswart · 2019-09-20T21:12:22Z

Library/ViewModels/AddNewCardViewModel.swift

@@ -39,6 +39,7 @@ public protocol AddNewCardViewModelOutputs {
  var saveButtonIsEnabled: Signal<Bool, Never> { get }
  var setStripePublishableKey: Signal<String, Never> { get }
  var zipcodeTextFieldBecomeFirstResponder: Signal<Void, Never> { get }
+  var newCardAdded: Signal<GraphUserCreditCard.CreditCard, Never> { get }


notifyDelegateNewCardAdded

justinswart · 2019-09-20T21:16:19Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift


 public typealias PledgePaymentMethodsValue = (user: User, project: Project, applePayCapable: Bool)

 public protocol PledgePaymentMethodsViewModelInputs {
+  func addNewCardSucceeded()


Looks like we don't use this input.

justinswart · 2019-09-20T21:24:26Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

  func applePayButtonTapped()
  func configureWith(_ value: PledgePaymentMethodsValue)
+  func didCreateCards(_ cards: [UIView])
+  func successfullyAddedCard(newCard: GraphUserCreditCard.CreditCard)


addNewCardViewControllerDidAdd(newCard card: GraphUserCreditCard.CreditCard)

justinswart · 2019-09-20T21:28:20Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

@@ -178,6 +193,16 @@ final class PledgePaymentMethodsViewController: UIViewController {
  }
 }

+extension PledgePaymentMethodsViewController: PledgeCreditCardViewDelegate {
+  func didSelectCard(_ cardView: PledgeCreditCardView) {
+    if let cards: [PledgeCreditCardView] = self.viewModel.outputs.savedCards() as? [PledgeCreditCardView] {


Instead of passing these views through the VM, let's keep references to them in an array in the VC. We can add them to that array when we create the views. That way we can just iterate over the array when selection changes and inform the view of the selected card. If that view is configured with that card then it can update its selected state.

justinswart · 2019-09-20T21:51:45Z

Also just noticed that there's a fair bit of overlap with #847 in terms of the card selection stuff, we might want to just sync up to align on that.

# Conflicts: # Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift # Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift # Library/ViewModels/CreditCardCellViewModel.swift # Library/ViewModels/CreditCardCellViewModelTests.swift # Library/ViewModels/PledgePaymentMethodsViewModel.swift

… tests, button selection

justinswart · 2019-09-23T22:14:33Z

ok, @cdolm92 take a look at the last set of changes i pushed here. I've added tests but take a look to see if you think I missed some.

We still need to confirm the correct values for greyButtonWhiteTextStyle.

…card-list-refresh

justinswart · 2019-09-25T02:49:43Z

Hey @cdolm92 just tested the dismiss on save - it looks like this dismisses in settings too, so I think what we'd need to do is instead dismiss the modal from the PledgeViewController based on the delegate saying that the call succeeded. Got held up with a few other things today so only managed to test this now, will catch up with you tomorrow about it.

justinswart

Cool lgtm!

dusi

Had couple suggestions and comments...I think the important thing to fix is making sure that we don't increment the amount of subviews in stack view's hierarchy.

dusi · 2019-09-25T20:59:36Z

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

@@ -70,16 +70,12 @@ final class PledgeCreditCardView: UIView {

  // MARK: - Styles

-  override func bindStyles() {
+  public override func bindStyles() {


Is public needed here? Are we exposing this to some other object?

dusi · 2019-09-25T21:00:00Z

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

  }

-  override func bindViewModel() {
+  public override func bindViewModel() {


Same as above (re: public)

dusi · 2019-09-25T21:03:12Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

+    self.viewModel.outputs.newCardAdded
+      .observeForUI()
+      .observeValues { [weak self] newCard in
+        guard let _self = self else { return }


Swift 4.2 allows us to simply do guard let self = self else { .. }. I know if you search the project for guard let _self = self there's 52 results as oppose to 27 when you do guard let self = self but I think that's because we've been moving towards self = self and haven't yet refactored the rest of the code base (good idea for an investment day or follow up PR).

Also this same PR uses both variations _self = self and self = self so one of the reasons we should probably go with the newer option (self = self).

What do you think?

Let's do a sweep as part of an investment day to establish a convention for this.

dusi · 2019-09-25T21:21:08Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

-        self?.addCardsToStackView(cards)
+        guard let self = self else { return }
+        self.scrollView.setContentOffset(.zero, animated: false)
+        self.addCardsToStackView(cards)


Since this operation will happen every time the payment methods refresh I'd suggest different naming ... using add implies that something is being appended ... how about updateStackViewWith(cards) or similar?

dusi · 2019-09-25T21:28:07Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

@@ -167,16 +176,24 @@ final class PledgePaymentMethodsViewController: UIViewController {
  private func addCardsToStackView(_ cards: [GraphUserCreditCard.CreditCard]) {
    self.cardsStackView.arrangedSubviews.forEach(self.cardsStackView.removeArrangedSubview)


There was recently an article on NSHipster - about stack views with an interesting tidbit - calling removeArrangedSubview does not really remove the subview from the superview so it still in the view hierarchy.

It should be safer iterating over subviews and calling .removeFromSuperview

See these 2 playgrounds and how the amount of subviews differ in the resolution panel.

Do not do it this way

But rather this way

Nice, thanks for the reminder about this!

dusi · 2019-09-25T21:42:07Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift


        return cardView
      }

-    let addNewCardView: UIView = PledgeAddNewCardView(frame: .zero)
+    self.cardViews = cardViews


Looks like the only reason for us to hold a reference to this list is for being able to iterate over an un-select all...which we could potentially even do like so:

self?.cardsStackView.arrangedSubviews.forEach { arrangedSubview in guard let cardView = arrangedSubview as? PledgeCreditCardView else { return } cardView.setSelectedCard(card) }

This would remove the need for the cardViews property altogether.

dusi · 2019-09-25T21:53:17Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

-    let cardViews: [UIView] = cards
+    let selectedCard = cards.first
+
+    let cardViews = cards


This method seems to be doing quite a bit..

removing previous subviews

creating new cells

appending add new card view

adding to view hierarchy

Any chance we can split these events into more digestible pieces or at least arrange them better?

So for example:

A) Better ordering

Create subview array (card views + add new card)

Remove previous views from hierarchy

Add new views to hierarchy

B) Extracted

private func creditCardViews(for: cards) -> [PledgeCreditCardView] - this is where the construction happens

`private func cardViews

private func creditCardViews(for: cards) -> [PledgeCreditCardView] { return cards .map { card -> PledgeCreditCardView in let cardView = PledgeCreditCardView(frame: .zero) cardView.delegate = self cardView.configureWith(value: card) if let selectedCard = selectedCard { cardView.setSelectedCard(selectedCard) } return cardView } }

private func cardViews(for: cards) -> [UIView] { let creditCardViews = self.creditCardViews(for: cards) let addCreditCardView = PledgeAddNewCardView = PledgeAddNewCardView(frame: .zero) |> \.delegate .~ self return creditCardViews.append(addCreditCardView) // or creditCardViews + [addCreditCardView] }

private func updateStackView(with cards: [GraphUserCreditCard.CreditCard]) { // Create views let cardViews = self.cardViews(for: cards) // Remove previous views from hierarchy self.cardsStackView.arrangedStackView.forEach { subview in subview.removeFromSuperview } // Add new views cardViews.forEach { cardView in self.cardsStackView.addArrangedSubview(cardView) } }

Ok, broke this up a bit, not quite as much but that'll do 😄. Good suggestion.

dusi · 2019-09-25T22:00:12Z

Library/ViewModels/AddNewCardViewModel.swift

@@ -35,6 +35,7 @@ public protocol AddNewCardViewModelOutputs {
  var creditCardValidationErrorContainerHidden: Signal<Bool, Never> { get }
  var cardholderNameBecomeFirstResponder: Signal<Void, Never> { get }
  var dismissKeyboard: Signal<Void, Never> { get }
+  var newCardAdded: Signal<GraphUserCreditCard.CreditCard, Never> { get }


In order for us to make this more readable, maybe we can define a typealias..

Currently there are 2 CreditCard structs

GraphUserCreditCard.CreditCard

Query.User.CreditCard

How about we declare typealias public typealias GraphCreditCard = GraphUserCreditCard.CreditCard?

Sounds good, let's not do that as part of this PR though 👍

dusi

🚢 🚢 🚢
🚢 🚢 🚢
🚢 🚢 🚢

…card-list-refresh

cdolm92 added 5 commits September 11, 2019 13:27

wip- rough but working, refactoring now, tests next

17f01d1

wip- adding new card now

96a472d

tests

13d927a

swift format

5c38a27

not using success message

f07f378

cdolm92 changed the title ~~[ 💳Native Checkout] Refresh Card List~~ [ 💳 Native Checkout] Refresh Card List Sep 11, 2019

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

b66aee6

…card-list-refresh

cdolm92 changed the title ~~[ 💳 Native Checkout] Refresh Card List~~ [ 💳 Native Checkout ] Refresh Card List Sep 11, 2019

cdolm92 changed the title ~~[ 💳 Native Checkout ] Refresh Card List~~ [ 💳 Native Checkout ] Refresh Card List Sep 11, 2019

cdolm92 added 7 commits September 12, 2019 17:31

wip- delegate working

27cc077

wip- bug cta switches between selected and normal state

91430ae

wip- now appears first, nxt: remove ! unwrapping

d42e6d9

wip- now appears first, correctly

6515224

wip- select state correct

284110d

swift format

f4624ea

swift lint fix

0b32387

cdolm92 added 12 commits September 18, 2019 11:31

tests

f2404fe

testing card view

8c1250a

copy for selected state

cac86fa

cta copy and color

f55c03a

wip- fixed button selection functionality

88c94a7

select background color

30009bb

handling deselection

203a44e

refactor

3b1a7dd

alphabetize inputs

4936e4e

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

b197f57

…card-list-refresh

alphabetize/refactor after merge

dd5257b

swiftformat

bd1c470

cdolm92 marked this pull request as ready for review September 20, 2019 17:58

cdolm92 assigned justinswart Sep 20, 2019

dusi reviewed Sep 20, 2019

View reviewed changes

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift Outdated Show resolved Hide resolved

justinswart suggested changes Sep 20, 2019

View reviewed changes

justinswart mentioned this pull request Sep 20, 2019

[NT-303] Pledge Button #847

Merged

4 tasks

justinswart added 2 commits September 23, 2019 14:33

Refactor, split CreditCardCellViewModel in two, add new button style,…

281bda7

… tests, button selection

justinswart and others added 4 commits September 23, 2019 15:35

Add selected color to blackButtonStyle instead

f92a888

scroll view fix and dismiss

878c572

vm tests

2b3049e

refactor

c0186f8

cdolm92 requested a review from justinswart September 24, 2019 20:48

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

58db7c0

…card-list-refresh

fixed dismissal

62a2eb1

cdolm92 requested a review from dusi September 25, 2019 16:05

justinswart approved these changes Sep 25, 2019

View reviewed changes

dusi suggested changes Sep 25, 2019

View reviewed changes

dusi mentioned this pull request Sep 25, 2019

[NT-204] Manage pledge payment method section #851

Merged

4 tasks

justinswart added 2 commits September 25, 2019 16:09

Refactor and rename function that populates stackview

98e061f

Remove array property

0298d52

justinswart assigned dusi and unassigned justinswart Sep 25, 2019

Put back code accidentally removed

e06566c

dusi approved these changes Sep 26, 2019

View reviewed changes

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

7fccf04

…card-list-refresh

cdolm92 merged commit 4ef012b into master Sep 26, 2019

cdolm92 deleted the card-list-refresh branch September 26, 2019 15:47

justinswart mentioned this pull request Sep 26, 2019

[NT-262] Remember this card added to mutation #856

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ 💳 Native Checkout ] Refresh Card List #835

[ 💳 Native Checkout ] Refresh Card List #835

cdolm92 commented Sep 11, 2019 •

edited by dusi

Loading

ksr-ci-bot commented Sep 17, 2019 •

edited

Loading

justinswart left a comment

justinswart Sep 20, 2019

justinswart Sep 20, 2019

justinswart Sep 20, 2019

justinswart Sep 20, 2019

justinswart Sep 20, 2019

cdolm92 Sep 23, 2019

justinswart Sep 23, 2019

justinswart Sep 20, 2019

justinswart Sep 20, 2019

justinswart Sep 20, 2019

justinswart Sep 20, 2019

justinswart Sep 20, 2019 •

edited

Loading

justinswart commented Sep 20, 2019

justinswart commented Sep 23, 2019

justinswart commented Sep 25, 2019

justinswart left a comment

dusi left a comment

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

justinswart Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

justinswart Sep 25, 2019

dusi Sep 25, 2019

justinswart Sep 25, 2019

dusi Sep 25, 2019

justinswart Sep 25, 2019

dusi Sep 25, 2019

justinswart Sep 25, 2019

dusi left a comment

		@@ -67,6 +74,9 @@ final class PledgeCreditCardView: UIView {
		_ = self.selectButton
		\|> cardSelectButtonStyle

		@@ -167,16 +176,24 @@ final class PledgePaymentMethodsViewController: UIViewController {
		private func addCardsToStackView(_ cards: [GraphUserCreditCard.CreditCard]) {
		self.cardsStackView.arrangedSubviews.forEach(self.cardsStackView.removeArrangedSubview)

[ 💳 Native Checkout ] Refresh Card List #835

[ 💳 Native Checkout ] Refresh Card List #835

Conversation

cdolm92 commented Sep 11, 2019 • edited by dusi Loading

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

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

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

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 Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

justinswart commented Sep 20, 2019

justinswart commented Sep 23, 2019

justinswart commented Sep 25, 2019

justinswart left a comment

Choose a reason for hiding this comment

dusi 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

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

Choose a reason for hiding this comment

dusi left a comment

Choose a reason for hiding this comment

cdolm92 commented Sep 11, 2019 •

edited by dusi

Loading

ksr-ci-bot commented Sep 17, 2019 •

edited

Loading

justinswart Sep 20, 2019 •

edited

Loading