Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Dec 17, 2024
+503 −57
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7fda0e7
Add functionality for the Pledge Over Time option selected
jovaniks Dec 5, 2024
5169766
Fix Tests
jovaniks Dec 5, 2024
ed401ec
Adding project to make the currency convertions and increments table …
jovaniks Dec 11, 2024
eff7fa4
Test timestamp with constant
jovaniks Dec 11, 2024
b5aab6c
Format
jovaniks Dec 11, 2024
43bd0a5
PR feedback
jovaniks Dec 11, 2024
5db01cb
Merge branch 'main' into jluna/MBL-1815/plot-plan-selector-selected-s…
jovaniks Dec 11, 2024
2bcd664
minor nit
jovaniks Dec 11, 2024
8893498
PR Feedback
jovaniks Dec 17, 2024
8589d20
Merge branch 'main' into jluna/MBL-1815/plot-plan-selector-selected-s…
jovaniks Dec 17, 2024
d38cfa3
Merge branch 'main' into jluna/MBL-1815/plot-plan-selector-selected-s…
jovaniks Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions Kickstarter-iOS/Features/PledgeOverTime/Controller/PledgePaymentPlansViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ protocol PledgePaymentPlansViewControllerDelegate: AnyObject {
_ viewController: PledgePaymentPlansViewController,
didSelectPaymentPlan paymentPlan: PledgePaymentPlansType
)
func pledgePaymentPlansViewController(
_ viewController: PledgePaymentPlansViewController,
didTapTermsOfUseWith helpType: HelpType
)
}

final class PledgePaymentPlansViewController: UIViewController {
Expand Down Expand Up @@ -39,11 +43,11 @@ final class PledgePaymentPlansViewController: UIViewController {
self.pledgeInFullOption.delegate = self
self.pledgeOverTimeOption.delegate = self

self.rootStackView.addArrangedSubviews([
self.rootStackView.addArrangedSubviews(
self.pledgeInFullOption,
self.separatorView,
self.pledgeOverTimeOption
])
)
}

private func setupConstraints() {
Expand Down Expand Up @@ -82,11 +86,13 @@ final class PledgePaymentPlansViewController: UIViewController {

self.pledgeInFullOption.configureWith(value: PledgePaymentPlanOptionData(
type: .pledgeInFull,
selectedType: data.selectedPlan
selectedType: data.selectedPlan,
paymentIncrements: data.paymentIncrements
))
self.pledgeOverTimeOption.configureWith(value: PledgePaymentPlanOptionData(
type: .pledgeOverTime,
selectedType: data.selectedPlan
selectedType: data.selectedPlan,
paymentIncrements: data.paymentIncrements
))
}

Expand All @@ -97,6 +103,13 @@ final class PledgePaymentPlansViewController: UIViewController {

self.delegate?.pledgePaymentPlansViewController(self, didSelectPaymentPlan: paymentPlan)
}
self.viewModel.outputs.notifyDelegateTermsOfUseTapped
.observeForUI()
.observeValues { [weak self] helpType in
guard let self = self else { return }

self.delegate?.pledgePaymentPlansViewController(self, didTapTermsOfUseWith: helpType)
}
}

// MARK: - Configuration
Expand All @@ -115,6 +128,13 @@ extension PledgePaymentPlansViewController: PledgePaymentPlanOptionViewDelegate
) {
self.viewModel.inputs.didSelectPlanType(paymentPlanType)
}

func pledgePaymentPlansViewController(
_: PledgePaymentPlanOptionView,
didTapTermsOfUseWith helpType: HelpType
) {
self.viewModel.inputs.didTapTermsOfUse(with: helpType)
}
}

// MARK: Styles
Expand Down
27 changes: 25 additions & 2 deletions ...starter-iOS/Features/PledgeOverTime/Controller/PledgePaymentPlansViewControllerTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,43 @@ final class PledgePaymentPlansViewControllerTest: TestCase {
}

func testView_PledgeOverTimeSelected() {
let testIncrements = testPledgePaymentIncrement()
orthogonalCombos([Language.en], [Device.pad, Device.phone4_7inch]).forEach { language, device in
withEnvironment(language: language) {
let controller = PledgePaymentPlansViewController.instantiate()

let data = PledgePaymentPlansAndSelectionData(selectedPlan: .pledgeOverTime)
let data = PledgePaymentPlansAndSelectionData(
selectedPlan: .pledgeOverTime,
increments: testIncrements
)
controller.configure(with: data)

controller.pledgePaymentPlanOptionView(
PledgePaymentPlanOptionView(),
didSelectPlanType: .pledgeOverTime
)

let (parent, _) = traitControllers(device: device, orientation: .portrait, child: controller)
parent.view.frame.size.height = 400

self.scheduler.advance(by: .seconds(1))
self.scheduler.advance(by: .seconds(3))

assertSnapshot(matching: parent.view, as: .image, named: "lang_\(language)_device_\(device)")
}
}
}
}

private func testPledgePaymentIncrement() -> [PledgePaymentIncrement] {
var increments: [PledgePaymentIncrement] = []
var timeStamp = Date().timeIntervalSince1970
for _ in 1...4 {
timeStamp += 30 * 24 * 60 * 60
increments.append(PledgePaymentIncrement(
amount: PledgePaymentIncrementAmount(amount: 250.0, currency: "USD"),
scheduledCollection: timeStamp
))
}

return increments
}
Binary file modified ...tPlansViewControllerTest/testView_PledgeOverTimeSelected.lang_en_device_pad.png
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...wControllerTest/testView_PledgeOverTimeSelected.lang_en_device_phone4_7inch.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
133 changes: 130 additions & 3 deletions Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ protocol PledgePaymentPlanOptionViewDelegate: AnyObject {
_ optionView: PledgePaymentPlanOptionView,
didSelectPlanType paymentPlanType: PledgePaymentPlansType
)
func pledgePaymentPlansViewController(
_ optionView: PledgePaymentPlanOptionView,
didTapTermsOfUseWith helpType: HelpType
)
}

final class PledgePaymentPlanOptionView: UIView {
Expand All @@ -16,6 +20,8 @@ final class PledgePaymentPlanOptionView: UIView {
private lazy var titleLabel = { UILabel(frame: .zero) }()
private lazy var subtitleLabel = { UILabel(frame: .zero) }()
private lazy var selectionIndicatorImageView: UIImageView = { UIImageView(frame: .zero) }()
private lazy var termsOfUseButton: UIButton = { UIButton(frame: .zero) }()
private lazy var paymentIncrementsStackView: UIStackView = { UIStackView(frame: .zero) }()

private let viewModel: PledgePaymentPlansOptionViewModelType = PledgePaymentPlansOptionViewModel()

Expand All @@ -27,7 +33,7 @@ final class PledgePaymentPlanOptionView: UIView {
self.bindViewModel()
self.configureSubviews()
self.setupConstraints()
self.configureTapGesture()
self.configureTapGestureAndActions()
}

@available(*, unavailable)
Expand All @@ -43,7 +49,21 @@ final class PledgePaymentPlanOptionView: UIView {
self.contentView.addSubview(self.selectionIndicatorImageView)
self.contentView.addSubview(self.optionDescriptorStackView)

self.optionDescriptorStackView.addArrangedSubviews([self.titleLabel, self.subtitleLabel])
self.optionDescriptorStackView.addArrangedSubviews(
self.titleLabel,
self.subtitleLabel,
self.termsOfUseButton,
self.paymentIncrementsStackView
)

// TODO: add strings translations [MBL-1860](https://kickstarter.atlassian.net/browse/MBL-1860)
self.termsOfUseButton.setAttributedTitle(
NSAttributedString(
string: "See our Terms of Use",
attributes: [NSAttributedString.Key.font: UIFont.ksr_caption1()]
),
for: .normal
)
}

private func setupConstraints() {
Expand Down Expand Up @@ -85,11 +105,17 @@ final class PledgePaymentPlanOptionView: UIView {
])
}

private func configureTapGesture() {
private func configureTapGestureAndActions() {
self.addGestureRecognizer(UITapGestureRecognizer(
target: self,
action: #selector(self.onOptionTapped)
))

self.termsOfUseButton.addTarget(
self,
action: #selector(self.onTermsOfUseButtonTapped),
for: .touchUpInside
)
}

// MARK: - Styles
Expand All @@ -98,9 +124,13 @@ final class PledgePaymentPlanOptionView: UIView {
super.bindStyles()

applyOptionDescriptorStackViewStyle(self.optionDescriptorStackView)
self.optionDescriptorStackView.setCustomSpacing(0, after: self.subtitleLabel)

applyTitleLabelStyle(self.titleLabel)
applySubtitleLabelStyle(self.subtitleLabel)
applySelectionIndicatorImageViewStyle(self.selectionIndicatorImageView)
applyTermsOfUseStyle(self.termsOfUseButton)
applyPaymentIncrementsStackViewStyle(self.paymentIncrementsStackView)
}

// MARK: - View model
Expand All @@ -126,6 +156,25 @@ final class PledgePaymentPlanOptionView: UIView {

self.delegate?.pledgePaymentPlanOptionView(self, didSelectPlanType: paymentPlan)
}

self.viewModel.outputs.notifyDelegateTermsOfUseTapped
.observeForUI()
.observeValues { [weak self] helpType in
guard let self = self else { return }

self.delegate?.pledgePaymentPlansViewController(self, didTapTermsOfUseWith: helpType)
}

self.termsOfUseButton.rac.hidden = self.viewModel.outputs.termsOfUseButtonHidden
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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

self.paymentIncrementsStackView.rac.hidden = self.viewModel.outputs.paymentIncrementsHidden
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, works for me.


self.viewModel.outputs.paymentIncrements
.observeForUI()
.observeValues { [weak self] increments in
guard let self = self else { return }

self.setupIncrementsStackView(with: increments)
}
}

func configureWith(value: PledgePaymentPlanOptionData) {
Expand All @@ -141,20 +190,67 @@ final class PledgePaymentPlanOptionView: UIView {
@objc private func onOptionTapped() {
self.viewModel.inputs.optionTapped()
}

@objc private func onTermsOfUseButtonTapped() {
self.viewModel.inputs.termsOfUseTapped()
}

// MARK: - Functions

private func setupIncrementsStackView(with increments: [PledgePaymentIncrementFormatted]) {
increments.forEach { increment in
let incrementStackView = UIStackView()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

applyIncrementStackViewStyle(incrementStackView)

let titleLabel = UILabel()
applyIncrementTitleLabelStyle(titleLabel)
titleLabel.text = increment.title
Copy link
Contributor

Choose a reason for hiding this comment

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

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


let detailsStackView = UIStackView()
applyIncrementDetailsStackViewStyle(detailsStackView)

let dateLabel = UILabel()
applyDetailLabelStyle(dateLabel)
dateLabel.text = increment.scheduledCollection

let amountLabel = UILabel()
applyDetailLabelStyle(amountLabel)
amountLabel.text = increment.amount
amountLabel.setContentCompressionResistancePriority(.required, for: .horizontal)

detailsStackView.addArrangedSubviews(dateLabel, amountLabel)
incrementStackView.addArrangedSubviews(titleLabel, detailsStackView)

self.paymentIncrementsStackView.addArrangedSubview(incrementStackView)
}
}
}

// MARK: - Styles helper

private func applyDetailLabelStyle(_ label: UILabel) {
label.font = UIFont.ksr_footnote()
label.textColor = .ksr_support_400
label.textAlignment = .left
label.adjustsFontForContentSizeCategory = true
}

private func applyOptionDescriptorStackViewStyle(_ stackView: UIStackView) {
stackView.axis = .vertical
stackView.spacing = Styles.grid(1)
}

private func applyPaymentIncrementsStackViewStyle(_ stackView: UIStackView) {
stackView.axis = .vertical
stackView.spacing = Styles.grid(2)
}

private func applyTitleLabelStyle(_ label: UILabel) {
label.accessibilityTraits = UIAccessibilityTraits.header
label.adjustsFontForContentSizeCategory = true
label.numberOfLines = 0
label.font = UIFont.ksr_subhead().bolded
label.textColor = .ksr_black
}

private func applySubtitleLabelStyle(_ label: UILabel) {
Expand All @@ -168,3 +264,34 @@ private func applySubtitleLabelStyle(_ label: UILabel) {
private func applySelectionIndicatorImageViewStyle(_ imageView: UIImageView) {
imageView.contentMode = .center
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

config.baseForegroundColor = .ksr_create_700
return config
}()

button.contentHorizontalAlignment = .leading
button.setContentHuggingPriority(.required, for: .horizontal)
}

private func applyIncrementStackViewStyle(_ stackView: UIStackView) {
stackView.axis = .vertical
stackView.alignment = .leading
stackView.spacing = Styles.gridHalf(1)
}

private func applyIncrementDetailsStackViewStyle(_ stackview: UIStackView) {
stackview.axis = .horizontal
stackview.spacing = Styles.grid(6)
}

private func applyIncrementTitleLabelStyle(_ label: UILabel) {
label.font = UIFont.ksr_footnote().bolded
label.textColor = .ksr_black
label.textAlignment = .left
label.adjustsFontForContentSizeCategory = true
label.setContentCompressionResistancePriority(.required, for: .vertical)
}
8 changes: 8 additions & 0 deletions Kickstarter-iOS/Features/PledgeView/Controllers/NoShippingPledgeViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,4 +735,12 @@ extension NoShippingPledgeViewController: PledgePaymentPlansViewControllerDelega
// TODO: Implement the necessary functionality once the ticket [MBL-1853] is resolved
debugPrint("pledgePaymentPlansViewController:didSelectPaymentPlan: \(paymentPlan)")
}

func pledgePaymentPlansViewController(
_: PledgePaymentPlansViewController,
didTapTermsOfUseWith helpType: HelpType
) {
self.paymentMethodsViewController.cancelModalPresentation(true)
self.viewModel.inputs.termsOfUseTapped(with: helpType)
}
}
2 changes: 1 addition & 1 deletion Library/Extensions/UIStackView+Helper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ extension UIStackView {
/// - Parameter subviews: An array of `UIView` objects to be added as arranged subviews.
/// - Note: This method iterates through the provided views and calls `addArrangedSubview(_:)`
/// on each, adding them to the stack view in the order they appear in the array.
public func addArrangedSubviews(_ subviews: [UIView]) {
public func addArrangedSubviews(_ subviews: UIView...) {
subviews.forEach(self.addArrangedSubview)
}
}
19 changes: 18 additions & 1 deletion Library/ViewModels/NoShippingPledgeViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
.map { showPledgeOverTimeUI -> PledgePaymentPlansAndSelectionData? in
guard showPledgeOverTimeUI else { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

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

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


return PledgePaymentPlansAndSelectionData(selectedPlan: .pledgeInFull)
return PledgePaymentPlansAndSelectionData(
selectedPlan: .pledgeInFull,
increments: mockPledgePaymentIncrement()
)
}.skipNil()

self.pledgeAmountViewHidden = context.map { $0.pledgeAmountViewHidden }
Expand Down Expand Up @@ -1244,3 +1247,17 @@ private func pledgeAmountSummaryViewData(
rewardIsLocalPickup: rewardIsLocalPickup
)
}

private func mockPledgePaymentIncrement() -> [PledgePaymentIncrement] {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

var increments: [PledgePaymentIncrement] = []
var timeStamp = Date().timeIntervalSince1970
for _ in 1...4 {
timeStamp += 30 * 24 * 60 * 60
increments.append(PledgePaymentIncrement(
amount: PledgePaymentIncrementAmount(amount: 250.0, currency: "USD"),
scheduledCollection: timeStamp
))
}

return increments
}
Loading