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

[NT-404] No reward title #870

Merged
merged 7 commits into from
Oct 4, 2019
Merged

[NT-404] No reward title #870

merged 7 commits into from
Oct 4, 2019

Conversation

Scollaco
Copy link
Contributor

@Scollaco Scollaco commented Oct 3, 2019

πŸ“² What

  • Changed title of backed No Reward to reflect the design.

πŸ€” Why

Β―(Β°_o)/Β―
Abstract link

πŸ›  How

  • Title the title on the RewardCardViewModel
  • updated tests
  • generated new screenshots

πŸ‘€ See

Unbacked πŸ› Backed πŸ¦‹
Simulator Screen Shot - iPhone 8 - 2019-10-03 at 15 46 44 Simulator Screen Shot - iPhone 8 - 2019-10-03 at 15 52 42

βœ… Acceptance criteria

  • Unbacked NoReward should have Pledge without a reward as title and Back it because you believe in it. as description.
  • Backed NoReward should have You pledged without a reward as title and Thanks for bringing this project one step closer to becoming a reality. as description.

Note:

Since this PR is not merged yet, in order to see the backed NoReward please follow this steps:
On ProjectPamphletViewController.swift, changed the private func goToManageViewPledge(project: Project, reward: Reward, refTag _: RefTag?) to:

 private func goToManageViewPledge(project: Project, reward: Reward, refTag: RefTag?) {
    let vc = rewardsCollectionViewController(project: project, refTag: refTag)
    self.present(vc, animated: true)
  }

@Scollaco Scollaco requested review from dusi and cdolm92 October 3, 2019 20:04
@Scollaco Scollaco changed the title No reward title [NT-404] No reward title Oct 3, 2019
Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

Looks good, only had one suggestion for function name but should not block this PR.

@@ -181,17 +181,35 @@ private func backingReward(fromProject project: Project) -> Reward? {
.coalesceWith(.noReward)
}

private func rewardDescription(project: Project, reward: Reward) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to not repeat reward I'd suggest we rename this to something like localizedDescription(project: Project, reward: Reward) so that it could be called like so map(localizedDescription(project:reward:)). What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering whether this would be more readable, but as soon as I posted it it seemed harder to read πŸ€¦β€β™‚

let isBacking = project.personalization.isBacking
  let isNoReward = reward.isNoReward
  let userHasBacked = userIsBacking(reward: reward, inProject: project)

  switch (isBacking, isNoReward, userHasBacked) {
  case (false, true, true):
    return Strings.Thanks_for_bringing_this_project_one_step_closer_to_becoming_a_reality()
  case (false, false, _):
    return Strings.Back_it_because_you_believe_in_it()
  case (true, true, _):
    return Strings.Back_it_because_you_believe_in_it()
  default:
    return reward.description
  }

@@ -115,7 +115,7 @@ final class RewardCardViewModelTests: TestCase {

self.rewardTitleLabelHidden.assertValues([false])
self.rewardTitleLabelText.assertValues([
"Thank you for supporting this project."
"You pledged without a reward"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would easily fit one line now

Copy link
Contributor

@cdolm92 cdolm92 left a comment

Choose a reason for hiding this comment

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

Awesome work on this!! πŸ›³πŸ›³πŸ›³

@Scollaco Scollaco merged commit f278118 into master Oct 4, 2019
@Scollaco Scollaco deleted the no-reward-title branch October 4, 2019 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants