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

Scollaco · 2019-09-23T20:29:54Z

📲 What

Adds the selected payment method to the ManageViewPledgeViewController

Note

This PR doesn't include Apple Pay information. We don't have designs for that yet and this should be handled in a different PR

🤔 Why

This will allow the user, by visiting the Manage/View pledge screen, to see which payment method was used to back a project.

🛠 How

Created a new ManagePledgePaymentMethodView class
Added a new property paymentSource to the Backing model
Reused GraphUserCreditCard.CreditCard as paymentSource property type
Made the GraphUserCreditCard.CreditCard conform to Argo.Decodable. This was necessary in order to be able to reuse GraphUserCreditCard.CreditCard as paymentSource
Added and fixed tests

👀 See

Default 🐛	Dynamic type 🦋

♿️ Accessibility

Works with VoiceOver
Supports Dynamic Type

✅ Acceptance criteria

With Native Checkout Pledge View feature enabled:

Select a backed project and go Manage/View pledge screen. The Payment Method section should be visible
The Payment Method section should show the values of the card used to back the project

…ewPledgeViewController

….com/kickstarter/ios-oss into manage-pledge-payment-method-section

dusi

Had a first pass...mostly looking good..couple suggestions plus some weirdness around a11y bellow

VoiceOver
I wasn't able to focus on any of the textual fields in the payments section using VoiceOver.

Dynamic Type
Looks like the title of the section does not allow multiple lines and therefore on smaller devices it gets trimmed (see the screenshot)

dusi · 2019-09-25T22:25:28Z

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

@@ -102,6 +102,8 @@ final class PledgeCreditCardView: UIView {
      |> checkoutCardStackViewStyle
  }

+  // MARK: - View model


Can we also add // MARK: - Configuration??

dusi · 2019-09-25T22:29:18Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+  private lazy var rootStackView: UIStackView = { UIStackView(frame: .zero) }()
+  private lazy var titleLabel: UILabel = { UILabel(frame: .zero) }()
+
+  private let viewModel = CreditCardCellViewModel()


We've been inconsistent in this lately but shouldn't we define vm as CreditCardCellViewModelTypebefore we initialize it? This way we will be told by the compiler if we try to access any variables directly (skippinginputs/outputs`).

Could you maybe also add refactoring ticket in JIRA to do the same for all the other 17 occurrences of let viewModel = where we let the compiler infer the type without being explicitly told so?

dusi · 2019-09-25T22:29:59Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+
+    self.configureViews()
+    self.setupConstraints()
+    self.bindViewModel()


Any update on this @justinswart ? Thought we talked about swizzling some swizzlers to make this automatic :)

dusi · 2019-09-25T22:30:59Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+
+  private func configureViews() {
+    _ = self
+      |> \.accessibilityElements .~ self.subviews


Should we exclude the image view here?

dusi · 2019-09-25T22:32:10Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+
+    self.expirationDateLabel.rac.text = self.viewModel.outputs.expirationDateText
+    self.lastFourLabel.rac.text = self.viewModel.outputs.cardNumberTextShortStyle
+    self.viewModel.outputs.cardImage


Can we add empty line here to separate rac and output signals?

On a second thought, would it be worth it adding reactive extension on the UIImageView to being able to bind image property? 🤔

That's a good idea. I can create a ticket for that to be done in a different PR.

dusi · 2019-09-25T22:38:11Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+    |> checkoutTitleLabelStyle
+    |> \.font .~ UIFont.ksr_caption1().bolded
+    |> \.adjustsFontForContentSizeCategory .~ true
+    |> \.textColor .~ .ksr_text_dark_grey_500


Can we be explicit when defining styles and use full types? UIColor. this helps the compiler with the type inference and prevents from potential type-check warnings. This has been something we've mostly seen with styles so this should only be done when dealing with styles. We can be more on ease for other operations.

dusi · 2019-09-25T22:41:02Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+    |> \.textColor .~ .ksr_text_dark_grey_500
+}
+
+private let cardLabelsStackViewStyle: StackViewStyle = { stackView in


Can we simply use verticalStackViewStyle without defining a wrapper style?

dusi · 2019-09-25T22:41:39Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+    |> \.textColor .~ UIColor.black
+    |> \.font .~ UIFont.ksr_subhead().bolded
+    |> \.adjustsFontForContentSizeCategory .~ true
+    |> \.text %~ { _ in Strings.Payment_method() }


I think we'll also need to use numberOfLines .~ 0 here to prevent from trimming the title

dusi · 2019-09-25T22:42:50Z

Kickstarter-iOS/Views/Controllers/ManageViewPledgeViewController.swift

@@ -114,7 +109,9 @@ final class ManageViewPledgeViewController: UIViewController {

    self.viewModel.outputs.configurePaymentMethodView
      .observeForUI()
-      .observeValues { _ in }
+      .observeValues { [weak self] in


Can we name the closure variable for better readability? To better describe $0 in this case. Feel like we've been pretty consistent about that everywhere else.

dusi · 2019-09-25T22:46:09Z

Library/ViewModels/ManageViewPledgeViewModel.swift

@@ -77,7 +79,7 @@ public final class ManageViewPledgeViewModel:
    self.viewDidLoadObserver.send(value: ())
  }

-  public let configurePaymentMethodView: Signal<Project, Never>
+  public let configurePaymentMethodView: Signal<GraphUserCreditCard.CreditCard, Never>


Similar to my comment here #835 (comment)

Would communicate the change with Christella to prevent from merge conflicts

So was the typealias created yet? I think that creating it in this PR will add a ton of new diffs, though.

…method-section

dusi

Still noticed couple things we should address plus I'm not sure if this is caused by this PR (I could not seem to find any traces of what it would cause)...but I'm unable to display the blue Manage CTA button when I go to live projects that I've backed...what's working on master isn't working on this branch..any ideas?

I only see this ¯_(ツ)_/¯

dusi · 2019-09-27T21:32:58Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+  label
+    |> checkoutTitleLabelStyle
+    |> \.font .~ UIFont.ksr_caption1().bolded
+    |> \.adjustsFontForContentSizeCategory .~ true


Duplicate of checkoutTitleLabelStyle

Suggested change

|> \.adjustsFontForContentSizeCategory .~ true

dusi · 2019-09-27T21:33:26Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+  label
+    |> checkoutTitleLabelStyle
+    |> \.font .~ UIFont.ksr_subhead().bolded
+    |> \.adjustsFontForContentSizeCategory .~ true


Duplicate of checkoutTitleLabelStyle

Suggested change

|> \.adjustsFontForContentSizeCategory .~ true

dusi · 2019-09-27T21:38:06Z

Kickstarter-iOS/Views/Controllers/ManagePledgeViewController.swift

+    let childViews: [UIView] = [
+      self.pledgeSummaryView,
+      self.paymentMethodView,
+      self.rewardReceivedViewController.view


I think we need to add this not only to view hierarchy but to view-controller hierarchy as we do at other places (this is to ensure proper behavior fo the view-controller when being embedded in a parent VC.

self.addChild(viewController) self.view.addSubview(viewController.view) // or using our functional helper viewController.didMove(toParent: self)

dusi · 2019-09-27T21:48:29Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+  private lazy var cardImageView: UIImageView = { UIImageView(frame: .zero) }()
+  private lazy var cardLabelsStackView: UIStackView = { UIStackView(frame: .zero) }()
+  private lazy var expirationDateLabel: UILabel = { UILabel(frame: .zero) }()
+  private lazy var lastFourLabel: UILabel = { UILabel(frame: .zero) }()


lastFour what? Should we be more explicit? Also the signal & the UI element do not match (not sure if this is again due to copy-pasting stuff around)?

dusi · 2019-09-27T21:50:06Z

Kickstarter-iOS/Views/Controllers/ManagePledgePaymentMethodView.swift

+
+private let cardExpirationDateLabelStyle: LabelStyle = { label in
+  label
+    |> checkoutTitleLabelStyle


Using checkoutTitleLabelStyle means we're giving it a headline trait...which we probably only want to do for the title section.

Currently, title is missing headline trait

While the two labels do have it (they probably shouldn't)

* Rename label and styles * Refactor styles * Add vc to parent properly * Flatten the style

dusi

Note: Since @Scollaco was away I've managed to address my own feedback and have it approved by @cdolm92

Scollaco added 7 commits September 20, 2019 13:18

Added ManagePledgePaymentMethodView, created PaymentSource type

68ef47f

Added ManagePledgePaymentMethodView, created PaymentSource type

05cb1d2

Fixed CreditCard decoding bug / View styling / Added view to ManageVi…

b00b1d6

…ewPledgeViewController

Merge conflicts

b124a24

Fixed / incremented tests

4786c87

Code cleanup / Added missing tests

786649e

Swiftformat

48ed87c

Scollaco requested review from dusi and ifbarrera September 23, 2019 20:29

Scollaco assigned dusi Sep 23, 2019

Scollaco added 5 commits September 23, 2019 16:30

Merge branch 'master' into manage-pledge-payment-method-section

236fb92

Alphabetization

e2f6694

Merge branch 'manage-pledge-payment-method-section' of https://github…

49c7f10

….com/kickstarter/ios-oss into manage-pledge-payment-method-section

Merge branch 'master' into manage-pledge-payment-method-section

6a10fd5

Fixed margins of pledge summary view / Updated configureVIews function

3d46aa8

dusi suggested changes Sep 25, 2019

View reviewed changes

Scollaco added 9 commits September 27, 2019 11:56

Merge remote-tracking branch 'oss/master' into manage-pledge-payment-…

576ba5b

…method-section

Set numberOfLines to 0 for title label

438100f

Renamed stackView / Used PledgeCreditCardViewModel to bind card infos

ce8263a

Added explicit object type while defining styles

60d5a66

Added closure variable name / removed unecessary style wrapper

926eeaa

Removed line that broke VoiceOver

aca4598

Reverted commented out line

41c1d57

Swiftformat

90772ea

Merge branch 'master' into manage-pledge-payment-method-section

d1a0c1b

dusi reviewed Sep 27, 2019

View reviewed changes

dusi mentioned this pull request Sep 30, 2019

[NT-204] Additional fixes #861

Merged

3 tasks

dusi added 2 commits October 1, 2019 08:59

[NT-204] Additional fixes (#861)

dafbf54

* Rename label and styles * Refactor styles * Add vc to parent properly * Flatten the style

Merge branch 'master' into manage-pledge-payment-method-section

3200e23

dusi approved these changes Oct 1, 2019

View reviewed changes

dusi merged commit 4d936fb into master Oct 1, 2019

dusi deleted the manage-pledge-payment-method-section branch October 1, 2019 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Scollaco commented Sep 23, 2019 •

edited by dusi

Loading

dusi left a comment

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

Scollaco Sep 27, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

dusi Sep 25, 2019

Scollaco Sep 27, 2019

dusi left a comment

dusi Sep 27, 2019

dusi Sep 27, 2019

dusi Sep 27, 2019

dusi Sep 27, 2019

dusi Sep 27, 2019

dusi left a comment

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

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

Conversation

Scollaco commented Sep 23, 2019 • edited by dusi Loading

📲 What

Note

🤔 Why

🛠 How

👀 See

♿️ Accessibility

✅ Acceptance criteria

With Native Checkout Pledge View feature enabled:

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

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

dusi left a comment

Choose a reason for hiding this comment

Scollaco commented Sep 23, 2019 •

edited by dusi

Loading