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 1 commit
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
60 changes: 43 additions & 17 deletions Kickstarter-iOS/Features/PledgeOverTime/Views/PledgePaymentPlanOptionView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
import Library
import UIKit

private enum Constants {
/// Spacing & Padding
public static let contentInsets = NSDirectionalEdgeInsets(top: 1.0, leading: 0, bottom: 1.0, trailing: 0)
public static let defaultPaddingSpacing = Styles.grid(2)
public static let detailsStackViewSpacing = Styles.grid(6)
public static let incrementStackViewSpacing = Styles.gridHalf(1)
public static let optionDescriptorStackViewSpacing = Styles.grid(1)

/// Size
public static let selectionIndicatorImageWith = Styles.grid(4)
}

protocol PledgePaymentPlanOptionViewDelegate: AnyObject {
func pledgePaymentPlanOptionView(
_ optionView: PledgePaymentPlanOptionView,
Expand Down Expand Up @@ -78,16 +90,28 @@ final class PledgePaymentPlanOptionView: UIView {
self.subtitleLabel.setContentHuggingPriority(.required, for: .vertical)

NSLayoutConstraint.activate([
self.contentView.leadingAnchor.constraint(equalTo: self.leadingAnchor, constant: Styles.grid(2)),
self.contentView.trailingAnchor.constraint(equalTo: self.trailingAnchor, constant: -Styles.grid(2)),
self.contentView.topAnchor.constraint(equalTo: self.topAnchor, constant: Styles.grid(2)),
self.contentView.bottomAnchor.constraint(equalTo: self.bottomAnchor, constant: -Styles.grid(2))
self.contentView.leadingAnchor.constraint(
equalTo: self.leadingAnchor,
constant: Constants.defaultPaddingSpacing
),
self.contentView.trailingAnchor.constraint(
equalTo: self.trailingAnchor,
constant: -Constants.defaultPaddingSpacing
),
self.contentView.topAnchor.constraint(
equalTo: self.topAnchor,
constant: Constants.defaultPaddingSpacing
),
self.contentView.bottomAnchor.constraint(
equalTo: self.bottomAnchor,
constant: -Constants.defaultPaddingSpacing
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, looks good to me.

)
])

NSLayoutConstraint.activate([
self.optionDescriptorStackView.leadingAnchor.constraint(
equalTo: self.selectionIndicatorImageView.trailingAnchor,
constant: Styles.grid(2)
constant: Constants.defaultPaddingSpacing
),
self.optionDescriptorStackView.trailingAnchor.constraint(equalTo: self.contentView.trailingAnchor),
self.optionDescriptorStackView.topAnchor.constraint(equalTo: self.contentView.topAnchor),
Expand All @@ -97,12 +121,15 @@ final class PledgePaymentPlanOptionView: UIView {
NSLayoutConstraint.activate([
self.selectionIndicatorImageView.topAnchor.constraint(equalTo: self.contentView.topAnchor),
self.selectionIndicatorImageView.leadingAnchor.constraint(equalTo: self.contentView.leadingAnchor),
self.selectionIndicatorImageView.widthAnchor.constraint(equalToConstant: Styles.grid(4)),
self.selectionIndicatorImageView.widthAnchor
.constraint(equalToConstant: Constants.selectionIndicatorImageWith),
self.selectionIndicatorImageView.heightAnchor.constraint(
equalTo: self.titleLabel.heightAnchor,
multiplier: 1.0
)
])

self.termsOfUseButton.setContentHuggingPriority(.required, for: .horizontal)
}

private func configureTapGestureAndActions() {
Expand Down Expand Up @@ -203,9 +230,9 @@ final class PledgePaymentPlanOptionView: UIView {
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
let chargeNumberLabel = UILabel()
applyIncrementChargeNumberLabelStyle(chargeNumberLabel)
chargeNumberLabel.text = increment.incrementChargeNumber

let detailsStackView = UIStackView()
applyIncrementDetailsStackViewStyle(detailsStackView)
Expand All @@ -221,7 +248,7 @@ final class PledgePaymentPlanOptionView: UIView {
amountLabel.setContentCompressionResistancePriority(.required, for: .horizontal)

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

self.paymentIncrementsStackView.addArrangedSubview(incrementStackView)
}
Expand All @@ -238,13 +265,13 @@ final class PledgePaymentPlanOptionView: UIView {

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

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

Expand All @@ -271,26 +298,25 @@ private func applySelectionIndicatorImageViewStyle(_ imageView: UIImageView) {
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)
config.contentInsets = Constants.contentInsets
config.baseForegroundColor = .ksr_create_700
return config
}()

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

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

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

private func applyIncrementTitleLabelStyle(_ label: UILabel) {
private func applyIncrementChargeNumberLabelStyle(_ label: UILabel) {
label.font = UIFont.ksr_footnote().bolded
label.textColor = .ksr_black
label.textAlignment = .left
Expand Down
12 changes: 6 additions & 6 deletions Library/ViewModels/NoShippingPledgeViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,19 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin

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

self.pledgeOverTimeConfigData = self.showPledgeOverTimeUI
.filter { $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: personally, I'd prefer to make this more explicit like .filter { showUI in showUI == true } but that's a minor nit.

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

return PledgePaymentPlansAndSelectionData(
.map { _, project -> PledgePaymentPlansAndSelectionData in
PledgePaymentPlansAndSelectionData(
selectedPlan: .pledgeInFull,
increments: mockPledgePaymentIncrement(),
project: project
)
}.skipNil()
}

self.pledgeAmountViewHidden = context.map { $0.pledgeAmountViewHidden }
self.pledgeAmountSummaryViewHidden = Signal.zip(baseReward, context).map { baseReward, context in
Expand Down
11 changes: 6 additions & 5 deletions Library/ViewModels/PledgePaymentPlansOptionViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import ReactiveSwift
public typealias PledgePaymentPlanOptionData = (
Copy link
Contributor

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

Choose a reason for hiding this comment

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

  1. 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.
  2. This will eventually come from the API, correct? Or will this object be created from the API object?

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’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.

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

Expand All @@ -20,7 +21,7 @@ public typealias PledgePaymentIncrementAmount = (
)

public struct PledgePaymentIncrementFormatted: Equatable {
public var title: String
public var incrementChargeNumber: String
public var amount: String
public var scheduledCollection: String
}
Expand Down Expand Up @@ -85,7 +86,7 @@ public final class PledgePaymentPlansOptionViewModel:
data.paymentIncrements
.enumerated()
.map { index, increment in
getPledgePaymentIncrementFormatted(increment, at: index, project: data.project)
formattedPledgePaymentIncrement(increment, at: index, project: data.project)
}
}
.filter { !$0.isEmpty }
Expand Down Expand Up @@ -148,7 +149,7 @@ private func getSubtitleText(by type: PledgePaymentPlansType, isSelected: Bool)
}
}

private func getPledgePaymentIncrementFormatted(
private func formattedPledgePaymentIncrement(
_ increment: PledgePaymentIncrement,
at index: Int,
project: Project
Expand All @@ -169,7 +170,7 @@ extension PledgePaymentIncrementFormatted {
let projectCurrencyCountry = projectCountry(forCurrency: project.stats.currency) ?? project.country

// TODO: add strings translations [MBL-1860](https://kickstarter.atlassian.net/browse/MBL-1860)
self.title = "Charge \(index + 1)"
self.incrementChargeNumber = "Charge \(index + 1)"
self.amount = Format.currency(
increment.amount.amount,
country: projectCurrencyCountry,
Expand Down
1 change: 0 additions & 1 deletion Library/ViewModels/PledgePaymentPlansViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ public struct PledgePaymentPlansAndSelectionData {
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)
- [MBL-1816](https://kickstarter.atlassian.net/browse/MBL-1816)
*/

Expand Down