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.

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-1288] Update pledge CTA #2000

Merged
merged 8 commits into from Mar 26, 2024
Merged

[MBL-1288] Update pledge CTA #2000

merged 8 commits into from Mar 26, 2024

Conversation

ifosli
Copy link
Contributor

@ifosli ifosli commented Mar 26, 2024

📲 What

Update pledge CTA by adding a separate context for late pledges and showing the "pledge immediately" string in a late pledge context.

Note: the bolding is currently a little off. I have a pr that fixes that submitted but not yet in staging, so this will be fixed the next time we make strings.

👀 See

Jira

Logged out Logged in (new string)
image image

✅ Acceptance criteria

  • Pledge immediately string shows in late pledge context and does not show for normal pledges

@ifosli ifosli changed the title Update pledge cta [MBL-1288] Update pledge CTA Mar 26, 2024
@ifosli ifosli marked this pull request as ready for review March 26, 2024 16:08
@ifosli ifosli requested review from a team and amy-at-kickstarter and removed request for a team March 26, 2024 16:08
documentAttributes: nil
) 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.

Will the helper method simpleHtmlAttributedString do what you need, here? Or is the custom implementation required?

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 nice find! I looked for something like this but clearly not hard enough - I really wasn't expecting us to already handle strings like this.

return text
}

private func getBoldRangesFromHtmlTags(plainString: String, htmlString: String) -> [NSRange] {
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 should live in String+SimpleHTML or another utility file, feels too general purpose for the pledge container. (If it doesn't already exist elsewhere).

Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a comment

Choose a reason for hiding this comment

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

LGTM; left a few non-blocking questions. The most important one is: will having .latePledge as a context break our assumptions down the line when we need to edit a late pledge?

var currentString = htmlString as NSString
while currentString.contains("<b>") {
let startTagRange = currentString.range(of: "<b>")
let endTagRange = currentString.range(of: "</b>")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: will this code break/infinite loop if there's no closing </b>?

@@ -179,7 +179,7 @@ final class RewardAddOnSelectionViewController: UIViewController {
.observeValues { [weak self] data in
guard let self else { return }

if featurePostCampaignPledgeEnabled(), data.project.isInPostCampaignPledgingPhase {
if data.context == .latePledge {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

case update
case changePaymentMethod
case updateReward

var confirmationLabelHidden: Bool {
switch self {
case .fixPaymentMethod, .changePaymentMethod, .updateReward: return true
case .pledge, .update: return false
case .pledge, .latePledge, .update: return false
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you want to the update the reward of a late pledge? Is it .latePledge or .updateReward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't update rewards (or change payment method) for late pledges, afaik. Since you're charged immediately, your pledge is also locked in as soon as you make it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it.

@ifosli ifosli merged commit 1e45fe9 into main Mar 26, 2024
5 checks passed
@ifosli ifosli deleted the updatePledgeCTA branch March 26, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants