[MBL-1815] PLOT plan selector - selected state #2213

jovaniks · 2024-12-05T17:43:11Z

📲 What

This PR implements the "Pledge Over Time" (PLOT) selected state in the Payment Plan Selector. It allows backers to choose the PLOT option and view the corresponding payment increments, including charge dates and amounts.

🤔 Why

The PLOT payment option enables backers to split their pledge amount into four equal payments, improving affordability and flexibility. This change is needed to meet the requirements for the PLOT initiative and ensure proper functionality within the payment plan selector.

🛠 How

Added a selectable "Pledge Over Time" (PLOT) option in the Payment Plan Selector.
Implemented logic to display mock payment increment records:
- Included charge dates (PledgePaymentIncrement).
- Displayed amounts for each payment (PledgePaymentIncrement.amount).
Payment increments are displayed in the their own currency.
Adapted the UI to follow the provided design:
- Added explanatory text below the PLOT option.
- Included Terms of Use link.
Mock data was used as the API is not yet ready.

👀 See

Jira
Figma

✅ Acceptance criteria

All tests pass
Confirm that payment increments (dates and amounts) are displayed correctly.
Verify the Terms of Use link functions as expected.
Verify the full description is displayed when selected.

⏰ TODO

Connect the component to the real API once it becomes available to replace mock data.

jovaniks · 2024-12-05T18:05:09Z

.../PledgePaymentPlansViewControllerTest/testView_PledgeOverTimeSelected.lang_en_device_pad.png

The spacing issue shown here is due to the test locking the layout to a specific screen size, which might not fully represent the component's dynamic behavior. In actual implementation, this component adapts its size based on the content and available screen dimensions, so the extra space seen in the test is expected.

Can you link the ticket you created here for reference? We should update how we use snapshot tests if we make a habit of testing individual views instead of the entire screen. (I'm a big fan of testing individual views, for the record; we just don't do that currently)

I haven’t created a ticket for this yet, but I can definitely do so to track this issue and the potential update to how we approach snapshot testing. Let me know if that works!

jovaniks · 2024-12-09T19:11:47Z

Library/ViewModels/PledgePaymentPlansViewModelTest.swift

+    self.vm.inputs.viewDidLoad()
+
+    self.vm.inputs.configure(with: self.selectionData)
+    self.vm.inputs.didTapTermsOfUse(with: .terms)


@scottkicks I checked the PLOT web version and they are showing the current Terms of use web page.

…alignment issue

jovaniks · 2024-12-11T00:43:59Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+
+    if let firstDateLabel = dateLabels.first {
+      dateLabels.forEach { label in
+        label.widthAnchor.constraint(equalTo: firstDateLabel.widthAnchor).isActive = true


Intervals dates/amount alignment issue fixe

Explaining the issue: when the increments table mixed dates with one-digit days (4 Jan 2025) and two-digit days (14 Feb 2025), the amountLabel was misaligned. To fix it, a width constraint (all equal width) was added to each dateLabel.

Can you add this as a code comment?

Sure, added!

scottkicks

Looking good! Just some clean up comments for you to consider.

scottkicks · 2024-12-11T16:15:46Z

.../PledgePaymentPlansViewControllerTest/testView_PledgeOverTimeSelected.lang_en_device_pad.png

It looks like there may be some extra spacing after "Charge 1". I know we were having issues without iPad snapshots for this previously. If it's the same issue here, that's fine for now. I'd like to fix that, though, if possible.

Of course, we need to address how we handle snapshot tests. I’ll create a ticket for this purpose. You can follow this thread for more details.

scottkicks · 2024-12-11T16:19:33Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+        self.delegate?.pledgePaymentPlansViewController(self, didTapTermsOfUseWith: helpType)
+      }
+
+    self.termsOfUseButton.rac.hidden = self.viewModel.outputs.termsOfUseButtonHidden


Do we need this output? When the option isn't selected the entire PledgePaymentPlanOptionView will be hidden anyway, right? I don't think there's a case where the terms button won't be shown when everything else is.

Yes, we need it because the termsOfUseButton is part of the same stack view (optionDescriptorStackView) as the titleLabel, subtitleLabel, paymentIncrementsStackView, etc. Any recommendations or thoughts?

Ah I thought that there was a stackview just for the items that need to be hidden/shown at the correct time.

scottkicks · 2024-12-11T16:19:57Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+      }
+
+    self.termsOfUseButton.rac.hidden = self.viewModel.outputs.termsOfUseButtonHidden
+    self.paymentIncrementsStackView.rac.hidden = self.viewModel.outputs.paymentIncrementsHidden


Actually same question about this stack view as the one above.

Same as with the termsOfUseButton, I could use the same output to manage all the controls that should be displayed when Pledge Over Time is selected. However, I prefer this solution as it provides control at the level of each individual UI element. Thoughts?

Is there a case where one element for PLOT would be shown and not another? If they're always grouped together, controlling them with one output makes more sense to me. But I'm not as familiar with the designs as you both are!

There’s no current case where one element would show and not another, except for the Ineligible state. In that case, we display the badge but hide the increments, description, and terms of use button. Given this, I think it makes sense to handle this cleanup as part of a tech-debt ticket. Let me know if that works for you.

Yup, works for me.

scottkicks · 2024-12-11T16:23:54Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+      let titleLabel = UILabel()
+      applyIncrementTitleLabelStyle(titleLabel)
+      titleLabel.text = increment.title


nit: Can we rename this to something like chargeNumberLabel or something that makes it a bit clearer?

scottkicks · 2024-12-11T16:31:00Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+private func applyTermsOfUseStyle(_ button: UIButton) {
+  button.configuration = {
+    var config = UIButton.Configuration.borderless()
+    config.contentInsets = NSDirectionalEdgeInsets(top: 1.0, leading: 0, bottom: 1.0, trailing: 0)


nit: we could throw these inset values, and any others if we have any, in a Constants enum. Similarly to EstimatedShippingCheckoutView.swift

Got it, I’ll move these contentInsets values (and any similar ones like spacing things) to a Constants enum for consistency. Thanks for the suggestion!

scottkicks · 2024-12-11T16:33:18Z

Library/ViewModels/NoShippingPledgeViewModel.swift

@@ -94,14 +94,18 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin

    let backing = project.map { $0.personalization.backing }.skipNil()
    self.showPledgeOverTimeUI = project.signal
-      .map { ($0.isPledgeOverTimeAllowed ?? false) && featurePledgeOverTimeEnabled()
-      }
+      .map { _ in featurePledgeOverTimeEnabled() }


I think we can keep the project.isPledgeOverTimeAllowed check. Eventually, when this feature is fully rolled out and we clean up the feature flag we'll still only want to show PLOT UI if the project has isPledgeOverTimeAllowed set to true.

ouch my bad, I removed a bit for test, I'll rollback this validation

scottkicks · 2024-12-11T16:35:45Z

Library/ViewModels/NoShippingPledgeViewModel.swift


    self.pledgeOverTimeConfigData = self.showPledgeOverTimeUI
-      .map { showPledgeOverTimeUI -> PledgePaymentPlansAndSelectionData? in
+      .combineLatest(with: project)
+      .map { showPledgeOverTimeUI, project -> PledgePaymentPlansAndSelectionData? in
        guard showPledgeOverTimeUI else { return nil }


nit: this guard statement could be replaced with a .filter(showPledgeOverTimeUI). Then this output wouldn't need to return an optional PledgePaymentPlansAndSelectionData

scottkicks · 2024-12-11T16:36:59Z

Library/ViewModels/PledgePaymentPlansOptionViewModel.swift

 import ReactiveSwift

 public typealias PledgePaymentPlanOptionData = (
  type: PledgePaymentPlansType,
-  selectedType: PledgePaymentPlansType
+  selectedType: PledgePaymentPlansType,
+  paymentIncrements: [PledgePaymentIncrement], // TODO: replece with API model


Suggested change

paymentIncrements: [PledgePaymentIncrement], // TODO: replece with API model

paymentIncrements: [PledgePaymentIncrement], // TODO: replece with API model in [mbl-1838](https://kickstarter.atlassian.net/browse/MBL-1838)

Updated, thanks for adding the ticket ref

scottkicks · 2024-12-11T16:43:29Z

Library/ViewModels/PledgePaymentPlansOptionViewModel.swift

+  }
+}
+
+private func getPledgePaymentIncrementFormatted(


nit:

Suggested change

private func getPledgePaymentIncrementFormatted(

private func formattedPledgePaymentIncrement(

renamed, good suggestion

scottkicks · 2024-12-11T16:44:45Z

Library/ViewModels/PledgePaymentPlansViewModel.swift

  public var selectedPlan: PledgePaymentPlansType
+  public var paymentIncrements: [PledgePaymentIncrement]
+  public var project: Project
  /* TODO: add the necesary properties for the next states (PLOT Selected and Ineligible)
     - [MBL-1815](https://kickstarter.atlassian.net/browse/MBL-1815)


Are we be able to remove this comment now?

Sure, removed the line that is refer to the [MBL-1815]

nativeksr · 2024-12-11T18:50:50Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

…tate

scottkicks

👍 just one last minor suggestion from me

scottkicks · 2024-12-11T20:39:32Z

Library/ViewModels/NoShippingPledgeViewModel.swift


    self.pledgeOverTimeConfigData = self.showPledgeOverTimeUI
+      .filter { $0 }


nit: personally, I'd prefer to make this more explicit like .filter { showUI in showUI == true } but that's a minor nit.

amy-at-kickstarter

This mostly looks good to me. Left some comments for clarity and a few suggestions. The two that are blocking for me are: using structs instead of tuples, and showing mock data in code merged to main. Can you address those two before merging this?

amy-at-kickstarter · 2024-12-16T19:48:48Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+      ),
+      self.contentView.bottomAnchor.constraint(
+        equalTo: self.bottomAnchor,
+        constant: -Constants.defaultPaddingSpacing


One thing that's slightly confusing - there's a very old, almost completely unused file KsApi/Constants.swift. Can you either rename this to something specific to PLOT (like PLOTConstants) or delete the old KsApi/Constants.swift?

These Constants are scoped privately within this file, and they were actually suggested by Scott to keep things clean and localized. I’ll also check KsApi/Constants.swift and delete it if it’s not being used anywhere. Let me know if you think we should still move or rename these to something like PLOTConstants

Oh, it looks like KsApi/Constants.swif is being used in KsApi/mutations/PostCommentMutation.swift

Gotcha - so this is out of scope here. I'll clean up KsApi/Constants.swift in another PR for a quick win.

Sounds good, thanks for taking care of that! Let me know if there's anything else needed from my side regarding this.

Nope, looks good to me.

amy-at-kickstarter · 2024-12-16T19:51:02Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+      }
+
+    self.termsOfUseButton.rac.hidden = self.viewModel.outputs.termsOfUseButtonHidden
+    self.paymentIncrementsStackView.rac.hidden = self.viewModel.outputs.paymentIncrementsHidden


Is there a case where one element for PLOT would be shown and not another? If they're always grouped together, controlling them with one output makes more sense to me. But I'm not as familiar with the designs as you both are!

amy-at-kickstarter · 2024-12-16T19:54:23Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+  private func setupIncrementsStackView(with increments: [PledgePaymentIncrementFormatted]) {
+    var dateLabels: [UILabel] = []
+    increments.forEach { increment in
+      let incrementStackView = UIStackView()


This would be cleaner if it was all pulled out into its own subview, like

increments.forEach { increment in let incrementView = PledgePaymentIncrementView() incrementView.increment = increment

and then all the label and formatting logic could live there as well.

That makes sense, and it would definitely help clean things up. I propose handling this as part of a tech-debt ticket so we can prioritize unblocking the API tasks for now. Let me know if that works for you!

amy-at-kickstarter · 2024-12-16T19:54:57Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+
+    if let firstDateLabel = dateLabels.first {
+      dateLabels.forEach { label in
+        label.widthAnchor.constraint(equalTo: firstDateLabel.widthAnchor).isActive = true


Can you add this as a code comment?

amy-at-kickstarter · 2024-12-16T20:41:42Z

Library/ViewModels/NoShippingPledgeViewModel.swift

@@ -1244,3 +1248,17 @@ private func pledgeAmountSummaryViewData(
    rewardIsLocalPickup: rewardIsLocalPickup
  )
 }
+
+private func mockPledgePaymentIncrement() -> [PledgePaymentIncrement] {


Merging mock data into the main branch makes me nervous. It looks like it should be safe, since it's behind featurePledgeOverTimeEnabled(), but I'd worry about us making a mistake with the feature rollout and accidentally showing mock data to users on older versions of the app.

Can you put the mock code in a #if DEBUG macro block, or just comment it out when merging and return an empty array?

Good point, Amy. I think using #if DEBUG is the best approach here to ensure the mock data doesn’t accidentally appear in production. I’ll update the code accordingly and push the changes.

amy-at-kickstarter · 2024-12-16T20:46:06Z

Library/ViewModels/PledgePaymentPlansOptionViewModel.swift

@@ -1,13 +1,35 @@
+import Foundation
+import KsApi
 import ReactiveSwift

 public typealias PledgePaymentPlanOptionData = (


If you need a model object here, it should be a struct and not a tuple. That way auto-complete and compile errors work significantly better.

This will eventually come from the API, correct? Or will this object be created from the API object?

I’ve updated this to use a struct as suggested. In fact, some of these models have already been converted to struct in another PR. For point 2, only the paymentIncrements model will need to be updated to use the object from the API. The rest of the structure will remain as is for now.

amy-at-kickstarter · 2024-12-16T20:48:47Z

Library/ViewModels/PledgePaymentPlansOptionViewModel.swift

    self.subtitleLabelHidden = self.subtitleText.map { $0.isEmpty }

    self.notifyDelegatePaymentPlanOptionSelected = self.optionTappedProperty
      .signal
      .withLatest(from: configData)
      .map { $1.type }
+
+    let isPledgeOverTimeAndSelected = configData.map {
+      $0.type != .pledgeOverTime || $0.type != $0.selectedType


This statement compared to the variable name is confusing, should it be $0.type == .pledgeOverTime && $0.type == $0.selectedType?

Good catch, Amy! I think this initially had other conditions, but now your suggestion makes perfect sense. I’ll apply your recommendation as is.

amy-at-kickstarter · 2024-12-16T20:49:54Z

Library/ViewModels/PledgePaymentPlansOptionViewModel.swift

+  )
+}
+
+extension PledgePaymentIncrementFormatted {


This looks like it could live in its own file (and probably have a test or two)

Good point! I’ll suggest moving this into its own file and adding a tests as part of a tech-debt task, if that sounds good to you. Let me know if you'd prefer we tackle it sooner.

jovaniks

I've left replies addressing the feedback. For several points, I’ve suggested handling the proposed changes in tech-debt tickets to allow us to move forward with the API implementation work. Let me know if that approach works or if further adjustments are needed!

jovaniks · 2024-12-17T08:25:42Z

Library/ViewModels/PledgePaymentPlansOptionViewModel.swift

    self.subtitleLabelHidden = self.subtitleText.map { $0.isEmpty }

    self.notifyDelegatePaymentPlanOptionSelected = self.optionTappedProperty
      .signal
      .withLatest(from: configData)
      .map { $1.type }
+
+    let isPledgeOverTimeAndSelected = configData.map {
+      $0.type != .pledgeOverTime || $0.type != $0.selectedType


Good catch, Amy! I think this initially had other conditions, but now your suggestion makes perfect sense. I’ll apply your recommendation as is.

jovaniks · 2024-12-17T08:28:43Z

Library/ViewModels/PledgePaymentPlansOptionViewModel.swift

+  )
+}
+
+extension PledgePaymentIncrementFormatted {


Good point! I’ll suggest moving this into its own file and adding a tests as part of a tech-debt task, if that sounds good to you. Let me know if you'd prefer we tackle it sooner.

jovaniks · 2024-12-17T08:35:05Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+      }
+
+    self.termsOfUseButton.rac.hidden = self.viewModel.outputs.termsOfUseButtonHidden
+    self.paymentIncrementsStackView.rac.hidden = self.viewModel.outputs.paymentIncrementsHidden


There’s no current case where one element would show and not another, except for the Ineligible state. In that case, we display the badge but hide the increments, description, and terms of use button. Given this, I think it makes sense to handle this cleanup as part of a tech-debt ticket. Let me know if that works for you.

jovaniks · 2024-12-17T08:37:05Z

Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift

+  private func setupIncrementsStackView(with increments: [PledgePaymentIncrementFormatted]) {
+    var dateLabels: [UILabel] = []
+    increments.forEach { increment in
+      let incrementStackView = UIStackView()


That makes sense, and it would definitely help clean things up. I propose handling this as part of a tech-debt ticket so we can prioritize unblocking the API tasks for now. Let me know if that works for you!

amy-at-kickstarter

Thanks for the updates! LGTM. I filed a low-priority ticket for the other issues we discussed but aren't fixing now: https://kickstarter.atlassian.net/browse/MBL-1940

…tate

jovaniks added 2 commits December 5, 2024 02:23

Add functionality for the Pledge Over Time option selected

7fda0e7

Fix Tests

5169766

jovaniks requested review from ifosli and amy-at-kickstarter December 5, 2024 17:43

jovaniks self-assigned this Dec 5, 2024

jovaniks commented Dec 5, 2024

View reviewed changes

jovaniks commented Dec 9, 2024

View reviewed changes

jovaniks requested a review from scottkicks December 9, 2024 20:57

Adding project to make the currency convertions and increments table …

ed401ec

…alignment issue

jovaniks commented Dec 11, 2024

View reviewed changes

jovaniks added 2 commits December 11, 2024 07:40

Test timestamp with constant

eff7fa4

Format

b5aab6c

scottkicks requested changes Dec 11, 2024

View reviewed changes

PR feedback

43bd0a5

jovaniks requested a review from scottkicks December 11, 2024 18:48

Merge branch 'main' into jluna/MBL-1815/plot-plan-selector-selected-s…

5db01cb

…tate

scottkicks approved these changes Dec 11, 2024

View reviewed changes

scottkicks mentioned this pull request Dec 11, 2024

[MBL-1816] PLOT Ineligible state #2220

Merged

4 tasks

minor nit

2bcd664

amy-at-kickstarter requested changes Dec 16, 2024

View reviewed changes

PR Feedback

8893498

jovaniks commented Dec 17, 2024

View reviewed changes

jovaniks requested a review from amy-at-kickstarter December 17, 2024 08:53

amy-at-kickstarter approved these changes Dec 17, 2024

View reviewed changes

jovaniks added 2 commits December 17, 2024 10:26

Merge branch 'main' into jluna/MBL-1815/plot-plan-selector-selected-s…

8589d20

…tate

Merge branch 'main' into jluna/MBL-1815/plot-plan-selector-selected-s…

d38cfa3

…tate

jovaniks merged commit 67c8b0e into main Dec 17, 2024
5 checks passed

jovaniks deleted the jluna/MBL-1815/plot-plan-selector-selected-state branch December 17, 2024 20:08

	paymentIncrements: [PledgePaymentIncrement], // TODO: replece with API model
	paymentIncrements: [PledgePaymentIncrement], // TODO: replece with API model in [mbl-1838](https://kickstarter.atlassian.net/browse/MBL-1838)

	private func getPledgePaymentIncrementFormatted(
	private func formattedPledgePaymentIncrement(


		self.pledgeOverTimeConfigData = self.showPledgeOverTimeUI
		.filter { $0 }

[MBL-1815] PLOT plan selector - selected state #2213

[MBL-1815] PLOT plan selector - selected state #2213

Conversation

jovaniks commented Dec 5, 2024 • 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

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

scottkicks 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

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

scottkicks Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nativeksr commented Dec 11, 2024

scottkicks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter 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

amy-at-kickstarter Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Dec 16, 2024 • 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

jovaniks 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

amy-at-kickstarter left a comment

Choose a reason for hiding this comment

jovaniks commented Dec 5, 2024 •

edited

Loading

scottkicks Dec 11, 2024 •

edited

Loading

amy-at-kickstarter Dec 16, 2024 •

edited

Loading

amy-at-kickstarter Dec 16, 2024 •

edited

Loading