[NT-205] Manage pledge reward section #868

Scollaco · 2019-10-03T15:24:21Z

📲 What

Adds the backed reward to the ManagePledge screen

🤔 Why

To allow users to see which reward they chose.

🛠 How

By creating a ManagePledgeRewardView containing basically the RewardCardView and a title label.
The RewardCardView now needs RewardCardViewContext to be configured properly, since little UI changes will occur depending if the reward is being seen from the Rewards Carousel or from the ManagePledgeViewController
Now that ManagePledgeViewController contains all the sections, snapshots were added

Note:

The No Reward title wasn't updated. This will be made in a small PR (coming soon) to avoid this PR to get too big.

👀 See

Trello, screenshots, external resources?

Default 🐛	Dynamic Type 🦋

♿️ Accessibility

Works with VoiceOver
Supports Dynamic Type

✅ Acceptance criteria

The backed reward should be shown on the ManagePledge screen below Payment Methods section.
The section title label should show Selected reward

…here the view is being presented from

…ection

ksr-ci-bot · 2019-10-03T15:31:19Z

	1 Warning
⚠️	Big PR

SwiftFormat found issues:

File	Rules
Kickstarter-iOS/Views/RewardCardView.swift	indent

Generated by 🚫 Danger

justinswart · 2019-10-07T17:00:00Z

Kickstarter-iOS/Views/Controllers/ManagePledgeViewController.swift

@@ -26,16 +24,51 @@ final class ManagePledgeViewController: UIViewController {
    )
  }()

-  private lazy var pledgeSummaryView: ManagePledgeSummaryView = { ManagePledgeSummaryView(frame: .zero) }()
+  private lazy var navigationBarShadowImage: UIImage? = {


This doesn't seem to be referenced anywhere?

(@justinswart ) This should be remove it since it's not being used. (Let's use a holistic approach to make the codebase consistent)

justinswart · 2019-10-07T20:24:37Z

Kickstarter-iOS/Views/RewardCardView.swift

@@ -277,11 +275,14 @@ public final class RewardCardView: UIView {
    self.setNeedsLayout()
  }

-  fileprivate func load(items: [String]) {
+  fileprivate func load(items: ([String], UIColor)) {


Could we name the tuple values here like fileprivate func load(items: (includedItems: [String], separatorBackgroundColo: UIColor)) {, and then below refer to like items.includedItems and items.separatorBackgroundColor?

justinswart · 2019-10-07T20:25:52Z

Kickstarter.xcodeproj/project.pbxproj

@@ -4329,7 +4329,7 @@
 				};
 			};
 			buildConfigurationList = A7E06C741C5A6EB300EBDCC2 /* Build configuration list for PBXProject "Kickstarter" */;
-			compatibilityVersion = "Xcode 3.2";
+			compatibilityVersion = "Xcode 10.0";


Hmm what dis?

Could we revert this line just because it seems unrelated?

justinswart · 2019-10-07T20:28:26Z

Library/ViewModels/RewardCardViewModel.swift

@@ -107,6 +123,14 @@ public final class RewardCardViewModel: RewardCardViewModelType, RewardCardViewM
            : rewardsItem.item.name
        }
      }
+      .combineLatest(with: context)


Ah cool, I don't use .combineLatest(with:) often enough 🤔

Scollaco · 2019-10-07T20:36:24Z

Kickstarter-iOS/Views/Controllers/ManagePledgeRewardView.swift

+  // MARK: - Configuration
+
+  public func configure(with value: (Project, Either<Reward, Backing>), context: RewardCardViewContext) {
+    self.rewardView.configure(with: value, context: context)


(@justinswart )This view can pass the context itself

Scollaco · 2019-10-07T20:40:06Z

Kickstarter-iOS/Views/Controllers/ManagePledgeViewController.swift

@@ -119,7 +168,9 @@ final class ManagePledgeViewController: UIViewController {

    self.viewModel.outputs.configureRewardSummaryView
      .observeForUI()
-      .observeValues { _ in }
+      .observeValues { [weak self] in
+        self?.rewardView.configure(with: $0, context: .pledgeView)


(@justinswart ) We don't need to pass the context here either

Scollaco · 2019-10-07T20:48:59Z

Kickstarter-iOS/Views/RewardCardView.swift

@@ -38,7 +38,7 @@ public final class RewardCardView: UIView {
        sectionInset: UIEdgeInsets(topBottom: Styles.grid(1))
      )
    )
-      |> \.backgroundColor .~ UIColor.white
+      |> \.backgroundColor .~ self.backgroundColor


(@justinswart ) Let's move this line to bindStyles()

Scollaco · 2019-10-07T20:55:31Z

Library/ViewModels/RewardCardViewModel.swift

@@ -99,6 +111,10 @@ public final class RewardCardViewModel: RewardCardViewModelType, RewardCardViewM

    self.includedItemsStackViewHidden = rewardItemsIsEmpty.skipRepeats()

+    let context = self.projectAndRewardOrBackingProperty.signal
+      .skipNil()
+      .map { $0.2 }


(@justinswart ) We can use .map(third) here

justinswart

I think this is looking good to go, one thing I'd like to just try to reverting the snapshots and trying to test against them again, I'm still not understanding why it's recorded new ones because there shouldn't be any change. Maybe the snapshot diff could tell us?

justinswart · 2019-10-07T23:29:09Z

Kickstarter.xcodeproj/project.pbxproj

@@ -4329,7 +4329,7 @@
 				};
 			};
 			buildConfigurationList = A7E06C741C5A6EB300EBDCC2 /* Build configuration list for PBXProject "Kickstarter" */;
-			compatibilityVersion = "Xcode 3.2";
+			compatibilityVersion = "Xcode 10.0";


Could we revert this line just because it seems unrelated?

dusi · 2019-10-08T17:26:30Z

By accident I've noticed this weirdness...any idea what's going on in there?? 🤔

justinswart · 2019-10-08T17:57:44Z

Kickstarter-iOS/Views/RewardCardView.swift

@@ -160,6 +159,7 @@ public final class RewardCardView: UIView {
    self.pillCollectionView.rac.hidden = self.viewModel.outputs.pillCollectionViewHidden
    self.rewardTitleLabel.rac.hidden = self.viewModel.outputs.rewardTitleLabelHidden
    self.rewardTitleLabel.rac.text = self.viewModel.outputs.rewardTitleLabelText
+    self.includedItemsTitleLabel.rac.textColor = self.viewModel.outputs.sectionTitleLabelTextColor


Just noticed here that the output name doesn't match the element that's being set?

Good catch!

…ection

justinswart

Cool I think this is gtg. Could you merge master and also maybe delete these old screenshots? Looks like they were recorded with a spelling mistake and then not removed after it was fixed:

…ection

…kstarter/ios-oss into manage-pledge-reward-section

Scollaco added 12 commits September 24, 2019 15:27

Added rewardview to the ManagePledgeViewController

1bb0497

Now using RewardCardViewContext to set the right style depending on w…

e936062

…here the view is being presented from

Merge remote-tracking branch 'oss/master' into manage-pledge-reward-s…

90ee7bc

…ection

Fixed colors based on context / Added separators to Manage Pledge View

4981930

Merge remote-tracking branch 'oss/master' into manage-pledge-reward-s…

0bdc382

…ection

Created container class for RewardCard view

287bf14

Fixed tests

2f2dc67

Merge remote-tracking branch 'oss/master' into manage-pledge-reward-s…

4864106

…ection

UI tweaks

58cad5a

SwiftFormat

fb41bed

View Controller tests

6cccece

Reverted unwanted change

0ee1d4f

Scollaco requested review from ifbarrera and justinswart October 3, 2019 15:24

Scollaco assigned justinswart Oct 3, 2019

Scollaco mentioned this pull request Oct 3, 2019

[NT-404] No reward title #870

Merged

2 tasks

justinswart reviewed Oct 7, 2019

View reviewed changes

Scollaco commented Oct 7, 2019

View reviewed changes

justinswart reviewed Oct 7, 2019

View reviewed changes

justinswart reviewed Oct 8, 2019

View reviewed changes

Scollaco added 5 commits October 8, 2019 14:14

Removed unnecessary argument from configureWith function

ee81a76

Removed navigationBarShadowImage property

a70f188

Moved backgroundColor func to bindStyles, renamed private function

68f2c1a

Changed map syntax

0c986b0

Reverted project compatibilityVersion

c60b1e1

Merge remote-tracking branch 'oss/master' into manage-pledge-reward-s…

69b1e7b

…ection

Scollaco force-pushed the manage-pledge-reward-section branch from 31ca55d to 69b1e7b Compare October 8, 2019 18:40

Renamed output

9e71ca2

justinswart approved these changes Oct 8, 2019

View reviewed changes

Scollaco added 6 commits October 9, 2019 10:38

Deleted snapshots with spelling mistake

6a03c3d

Merge remote-tracking branch 'oss/master' into manage-pledge-reward-s…

99bb8ab

…ection

Merge branch 'master' into manage-pledge-reward-section

a092622

Merge remote-tracking branch 'oss/master' into manage-pledge-reward-s…

0503111

…ection

Merge branch 'manage-pledge-reward-section' of https://github.com/kic…

5c0e11e

…kstarter/ios-oss into manage-pledge-reward-section

Merge branch 'master' into manage-pledge-reward-section

63f8ab1

Scollaco merged commit c1a2d59 into master Oct 9, 2019

Scollaco deleted the manage-pledge-reward-section branch October 9, 2019 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-205] Manage pledge reward section #868

[NT-205] Manage pledge reward section #868

Scollaco commented Oct 3, 2019

ksr-ci-bot commented Oct 3, 2019 •

edited

Loading

justinswart Oct 7, 2019

Scollaco Oct 7, 2019

justinswart Oct 7, 2019

justinswart Oct 7, 2019

justinswart Oct 7, 2019

justinswart Oct 7, 2019

Scollaco Oct 7, 2019 •

edited

Loading

Scollaco Oct 7, 2019

Scollaco Oct 7, 2019

Scollaco Oct 7, 2019

justinswart left a comment

justinswart Oct 7, 2019

dusi commented Oct 8, 2019

justinswart Oct 8, 2019

Scollaco Oct 8, 2019

justinswart left a comment

[NT-205] Manage pledge reward section #868

[NT-205] Manage pledge reward section #868

Conversation

Scollaco commented Oct 3, 2019

📲 What

🤔 Why

🛠 How

Note:

👀 See

♿️ Accessibility

✅ Acceptance criteria

ksr-ci-bot commented Oct 3, 2019 • edited Loading

SwiftFormat found issues:

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

Scollaco Oct 7, 2019 • 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

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusi commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart left a comment

Choose a reason for hiding this comment

ksr-ci-bot commented Oct 3, 2019 •

edited

Loading

Scollaco Oct 7, 2019 •

edited

Loading