MBL-1456: Stub PPO view and container #2080

amy-at-kickstarter · 2024-06-12T20:28:56Z

📲 What

Stub code for our new PPO dashboard.

🤔 Why

PPO development is moving full steam ahead; this will give us a place to put the work we're doing.

🛠 How

This consists of a few separate pieces:

PagedContainerViewController and PagedContainerViewModel, a container UIViewController. It displays a segmented control (which will need to be replaced with the actual control). I realized I had to write this in UIKit, because the container needs to also hold the old ActivitiesViewController, and it seemed like it would be a bit of compatibility wizardry to get a SwiftUI view to contain a UIViewController.
I did discover while doing this the SortPagerViewController, which has the selector control we do want - but that'll have to be either abstracted out or re-written, since that view controller is fairly specific to the Discover flow.
PPOContainerViewController, a PagedContainerViewController which has tabs for the PPO view and Activity
A swift view called PPOView and matching PPOViewModel to contain all our PPO content
A stub test for PPOViewModel

amy-at-kickstarter · 2024-06-20T19:40:55Z

Library/PagedContainer/PagedContainerViewController.swift

+open class PagedContainerViewController: UIViewController {
+  private weak var activeController: UIViewController? = nil
+  private var subscriptions = Set<AnyCancellable>()
+  private let viewModel = PagedContainerViewModel()


Philosophically, I put everything that seemed like UIKit logic in the ViewController, and everything that seemed like "how it's displayed" logic in the view model. However, that ended up meaning the view model was very small since it's a pretty simple concept for a page 🤷‍♀️

amy-at-kickstarter · 2024-06-20T19:41:03Z

Library/PagedContainer/PagedContainerViewController.swift

@@ -0,0 +1,138 @@
+import Combine


amy-at-kickstarter · 2024-06-20T19:41:34Z

Library/PagedContainer/PagedContainerViewController.swift

+  private let viewModel = PagedContainerViewModel()
+
+  // TODO: Use the correct page control, per the designs.
+  // This may exist already in SortPagerViewController, or we can write one in SwiftUI.


Worth calling out - we have another ticket for implementing the correct page control. I leave it up to someone else to determine if we should write a new one in SwiftUI or steal/refactor out the existing one.

amy-at-kickstarter · 2024-06-20T19:44:02Z

Library/PagedContainer/PagedContainerViewController.swift

+    if self.toggle.selectedSegmentIndex == UISegmentedControl.noSegment {
+      self.viewModel.didSelectPage(atIndex: 0)
+    } else if let activeController = self.activeController {
+      activeController.beginAppearanceTransition(true, animated: animated)


OK, so all this elaborate appearanceTransition stuff. This exists so that the child view controllers will receive viewWillAppear with animated = true. The reason we need that is because ActivitiesViewController filters out calls that are unanimated; the result is that it was never showing its content when used as a child view controller.

I was going to clean up ActivitiesViewController, instead, but it turns out that we implicitly use that animated as a filter for whether or not the ActivitiesViewController is being instantiated by a screenshot test. Changing how we handle viewWillAppear in ActivitiesViewController then breaks those tests. It's unclear if we're also relying on that filter in other ways, but I didn't want to muck with something I don't understand yet.

Basically, it was rabbithole, and it seemed clearer to fix it this way.

Oh, I think that animated thing is what broke our surveys not showing up in activity, too. So it definitely might be worth looking into at some point but I agree that it's complicated and maybe not worth doing right now (and I definitely think it's not worth doing if there's a chance that we'll rewrite the view controller to stop basically being a webview).

I do also think it might be worth leaving a comment in the code about this; it doesn't need to be as detailed, though!

Good idea, I'll just pull in this comment and edit it a bit.

Library/PagedContainer/PagedContainerViewModel.swift

amy-at-kickstarter · 2024-06-20T20:53:50Z

Library/PagedContainer/PagedContainerViewModel.swift

+  // Inputs
+  func viewWillAppear() {
+    if self.selectedIndex.value == nil {
+      self.didSelectPage(atIndex: 0)


This tiny bit of state (in self.selectedIndex, a CurrentValueSubject instead of a straight PassthroughSubject), saves a lot of lines of confusing stateless code. I'm OK with that.

amy-at-kickstarter · 2024-06-20T20:55:27Z

Library/PagedContainer/PagedContainerViewModel.swift

+      controllers.map { $0.title ?? "Page " }
+    }.eraseToAnyPublisher()
+
+    self.displayChildViewControllerAtIndex = Publishers.CombineLatest(


I feel like this should really be a takeWhen but I can't find the Combine equivalent. We want the index + children when the index changes.

I don't think there is an equivalent? I don't think it matters for this one since I doubt the other signal can change. But if we really end up needing a takeWhen we may need to build our own. (Or, alternatively, store more state so that we don't need to grab everything from publishers?)

Fair point that the other signal (probably) won't ever change! The only situation I could see would be if we ever added a new page of content to this tab, and that page of content was affected by logged in/logged out state.

amy-at-kickstarter · 2024-06-20T20:56:22Z

Kickstarter-iOS/Features/PostPledgeOverview/PPOView.swift

@@ -0,0 +1,12 @@
+import SwiftUI


ifosli

Nice!! This feels like it's actually getting us started and I'm excited! Left a couple comments but everything looks pretty good apart from the "PostPledgeOverview" folder name being confusing (or at least new to me)

ifosli · 2024-06-21T21:53:42Z

Kickstarter-iOS/Features/PostPledgeOverview/PPOContainerViewController.swift

+  public override func viewDidLoad() {
+    super.viewDidLoad()
+
+    // TODO: Translate these strings


Nit: Do we have a ticket number to tag here?

Good idea, I'll add one.

ifosli · 2024-06-21T21:55:39Z

Kickstarter.xcodeproj/project.pbxproj

@@ -4768,6 +4780,7 @@
 				1937A6F028C92F0000DD732D /* PledgeAmountSummary */,
 				19A97D3928C802230031B857 /* PledgePaymentMethods */,
 				1937A70428C9392600DD732D /* PledgeShippingLocation */,
+				E1BAA0332C1A18DA004F8B06 /* PostPledgeOverview */,


I could be wrong, but I thought PPO stood for "Pledged Projects Overview" so I think PledgedProjectsOverview would make more sense as a folder name than PostPledgeOverview

Hahaha no you are correct, I keep mixing that up again and again 😆 Will fix.

ifosli · 2024-06-21T22:32:18Z

Library/PagedContainer/PagedContainerViewController.swift

+    if self.toggle.selectedSegmentIndex == UISegmentedControl.noSegment {
+      self.viewModel.didSelectPage(atIndex: 0)
+    } else if let activeController = self.activeController {
+      activeController.beginAppearanceTransition(true, animated: animated)


Oh, I think that animated thing is what broke our surveys not showing up in activity, too. So it definitely might be worth looking into at some point but I agree that it's complicated and maybe not worth doing right now (and I definitely think it's not worth doing if there's a chance that we'll rewrite the view controller to stop basically being a webview).

ifosli · 2024-06-21T22:33:27Z

Library/PagedContainer/PagedContainerViewController.swift

+    if self.toggle.selectedSegmentIndex == UISegmentedControl.noSegment {
+      self.viewModel.didSelectPage(atIndex: 0)
+    } else if let activeController = self.activeController {
+      activeController.beginAppearanceTransition(true, animated: animated)


I do also think it might be worth leaving a comment in the code about this; it doesn't need to be as detailed, though!

ifosli · 2024-06-21T22:49:34Z

Library/PagedContainer/PagedContainerViewModel.swift

+      controllers.map { $0.title ?? "Page " }
+    }.eraseToAnyPublisher()
+
+    self.displayChildViewControllerAtIndex = Publishers.CombineLatest(


I don't think there is an equivalent? I don't think it matters for this one since I doubt the other signal can change. But if we really end up needing a takeWhen we may need to build our own. (Or, alternatively, store more state so that we don't need to grab everything from publishers?)

ifosli · 2024-06-21T22:50:14Z

Library/PagedContainer/PagedContainerViewModel.swift

+  // Inputs
+  func viewWillAppear() {
+    if self.selectedIndex.value == nil {
+      self.didSelectPage(atIndex: 0)


amy-at-kickstarter force-pushed the feat/adyer/mbl-1456/wip branch from aa8f780 to 123db4b Compare June 20, 2024 12:48

amy-at-kickstarter changed the title ~~WIP: Stub PPO view and container~~ MBL-1456: Stub PPO view and container Jun 20, 2024

amy-at-kickstarter force-pushed the feat/adyer/mbl-1456/wip branch 2 times, most recently from 4615357 to 5a4eea7 Compare June 20, 2024 15:09

amy-at-kickstarter marked this pull request as ready for review June 20, 2024 19:38

amy-at-kickstarter requested review from a team and ifosli and removed request for a team June 20, 2024 19:38

amy-at-kickstarter self-assigned this Jun 20, 2024

amy-at-kickstarter added the pledged-projects-overview label Jun 20, 2024

amy-at-kickstarter commented Jun 20, 2024

View reviewed changes

Library/PagedContainer/PagedContainerViewController.swift

@@ -0,0 +1,138 @@

import Combine

Copy link

Contributor Author

amy-at-kickstarter Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

amy-at-kickstarter commented Jun 20, 2024

View reviewed changes

amy-at-kickstarter force-pushed the feat/adyer/mbl-1456/wip branch from cf9bc1c to b2203a5 Compare June 20, 2024 19:49

amy-at-kickstarter commented Jun 20, 2024

View reviewed changes

Library/PagedContainer/PagedContainerViewModel.swift Show resolved Hide resolved

amy-at-kickstarter marked this pull request as draft June 20, 2024 20:29

amy-at-kickstarter force-pushed the feat/adyer/mbl-1456/wip branch from b2203a5 to 0ba8143 Compare June 20, 2024 20:52

amy-at-kickstarter marked this pull request as ready for review June 20, 2024 20:52

amy-at-kickstarter commented Jun 20, 2024

View reviewed changes

Kickstarter-iOS/Features/PostPledgeOverview/PPOView.swift Outdated

@@ -0,0 +1,12 @@

import SwiftUI

Copy link

Contributor Author

amy-at-kickstarter Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

amy-at-kickstarter force-pushed the feat/adyer/mbl-1456/wip branch from 0ba8143 to 5c07ac6 Compare June 21, 2024 14:38

ifosli approved these changes Jun 21, 2024

View reviewed changes

MBL-1456: Stub PPO view and container

9d991ee

amy-at-kickstarter force-pushed the feat/adyer/mbl-1456/wip branch from 5c07ac6 to 9d991ee Compare June 24, 2024 14:41

Changes per review #2080

a636cef

amy-at-kickstarter merged commit 4c0ee6e into main Jun 24, 2024
5 checks passed

amy-at-kickstarter deleted the feat/adyer/mbl-1456/wip branch June 24, 2024 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBL-1456: Stub PPO view and container #2080

MBL-1456: Stub PPO view and container #2080

amy-at-kickstarter commented Jun 12, 2024 •

edited

Loading

amy-at-kickstarter Jun 20, 2024

amy-at-kickstarter Jun 20, 2024

amy-at-kickstarter Jun 20, 2024

amy-at-kickstarter Jun 20, 2024 •

edited

Loading

ifosli Jun 21, 2024

ifosli Jun 21, 2024

amy-at-kickstarter Jun 24, 2024

amy-at-kickstarter Jun 20, 2024

ifosli Jun 21, 2024

amy-at-kickstarter Jun 20, 2024

ifosli Jun 21, 2024

amy-at-kickstarter Jun 24, 2024

amy-at-kickstarter Jun 20, 2024

ifosli left a comment

ifosli Jun 21, 2024

amy-at-kickstarter Jun 24, 2024

ifosli Jun 21, 2024

amy-at-kickstarter Jun 24, 2024

ifosli Jun 21, 2024

ifosli Jun 21, 2024

ifosli Jun 21, 2024

ifosli Jun 21, 2024

MBL-1456: Stub PPO view and container #2080

MBL-1456: Stub PPO view and container #2080

Conversation

amy-at-kickstarter commented Jun 12, 2024 • edited Loading

📲 What

🤔 Why

🛠 How

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Jun 20, 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

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

ifosli 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

amy-at-kickstarter commented Jun 12, 2024 •

edited

Loading

amy-at-kickstarter Jun 20, 2024 •

edited

Loading