[NT-361] Select new reward polish #899

Scollaco · 2019-10-17T20:26:24Z

📲 What

Changes the title of the RewardsCollectionViewController based on the RewardsCollectionViewContext.
Centralizes horizontally the backed reward card when presenting screen from ManagePledgeViewController.

🤔 Why

To match the behavior and design shown here.

🛠 How

Creating RewardsCollectionViewContext enum. I thought of reusing RewardCardViewContext, but while I was developing this, I noticed that the code was getting harder to understand.
Adding output that emits title based of the context
Adding output that emits IndexPath of backed reward. This will allow the collection view to centralize the reward when presenting the carousel from the ManagePledge screen.
Adding and fixing tests

👀 See

Backing Project	Managing Pledge

✅ Acceptance criteria

Select a non-backed project and open the Rewards Carousel. The No reward should be centralized and the title should show Back this project
Select a backed and live project and open the Rewards Carousel by selecting Choose another reward from the update pledge menu. The collection view should centralize the backed reward and the title should show Choose another reward

# Conflicts: # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_de_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_de_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_es_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_es_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_fr_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_fr_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_ja_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_ja_device_phone4_7inch@2x.png

…e Pay

…tation

…-instead

…tation

# Conflicts: # Kickstarter-iOS/Views/Controllers/PledgeAmountSummaryViewController.swift # Library/PledgeViewContext.swift # Library/PledgeViewContextTests.swift # Library/ViewModels/PledgeViewModel.swift # Library/ViewModels/PledgeViewModelTests.swift # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_ChangePaymentMethodContext_lang_de_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_ChangePaymentMethodContext_lang_en_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_ChangePaymentMethodContext_lang_fr_device_phone4_7inch@2x.png

justinswart · 2019-10-21T16:33:45Z

Kickstarter-iOS/Views/Controllers/RewardsCollectionViewController.swift

+    self.viewModel.outputs.backedRewardIndexPath
+      .observeForUI()
+      .observeValues { [weak self] indexPath in
+        self?.collectionView.scrollToItem(at: indexPath, at: .centeredHorizontally, animated: true)


I think we want this to be animated: false or was it specified that it should animate over to the reward? I would've expect that the view would just appear centered on the reward that I backed.

justinswart · 2019-10-21T16:34:26Z

Library/ViewModels/RewardsCollectionViewModel.swift

    self.reloadDataWithValues = Signal.combineLatest(project, rewards)
      .map { project, rewards in
        rewards.map { (project, Either<Reward, Backing>.left($0)) }
      }

+    self.backedRewardIndexPath = Signal.combineLatest(project, rewards)
+      .takeWhen(self.viewDidAppearProperty.signal.ignoreValues())


Based on my other comment (if we want the centering to have happened before the view appears) we might want to tie this to viewWillAppear.

justinswart · 2019-10-21T16:35:12Z

Library/ViewModels/RewardsCollectionViewModel.swift

@@ -46,11 +53,21 @@ public final class RewardsCollectionViewModel: RewardsCollectionViewModelType,
    let rewards = project
      .map { $0.rewards }

+    self.title = configData
+      .map(third)
+      .takeWhen(self.viewDidLoadProperty.signal.ignoreValues())


We typically always want our configData to be combineLatest with viewDidLoad. This is just to ensure that we aren't concerned with the order in which these events occur.

# Conflicts: # Library/ViewModels/PledgeAmountViewModelTests.swift # Library/ViewModels/PledgeViewModelTests.swift # Library/ViewModels/RewardsCollectionViewModelTests.swift

Scollaco · 2019-10-23T18:57:02Z

Library/ViewModels/RewardsCollectionViewModel.swift

+      .map(titleForContext)
+
+    self.backedRewardIndexPath = Signal.combineLatest(project, rewards)
+      .takeWhen(self.viewDidLayoutSubviewsProperty.signal.ignoreValues())


Using viewWillAppearProperty here didn't do anything, so viewDidLayoutSubviews was the way to go.

Interesting 🤔 I believe it needs a run loop to render the cells. We could probably go ahead with this but for interest sake, if you used viewWillAppear and added .ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler) which is a delay of 0 and just puts the emission on the next run loop, that would also work.

justinswart

This mostly checks out! Had one more suggestion and then just want to test again once we're able to 'change to no reward'.

justinswart · 2019-10-23T23:39:00Z

Library/ViewModels/RewardsCollectionViewModel.swift

+  return context == .createPledge ? Strings.Back_this_project() : Strings.Choose_another_reward()
+}
+
+private func backedReward(_ project: Project, rewards: [Reward]) -> IndexPath? {


Please update this to use the function @ifbarrera is using in #905 when that merges.

https://github.com/kickstarter/ios-oss/pull/905/files#diff-234cfa58d101616352832cb332756878R33

justinswart · 2019-10-24T21:49:26Z

Library/ViewModels/RewardsCollectionViewModel.swift

  func viewDidLoad()
  func viewWillAppear()
 }

 public protocol RewardsCollectionViewModelOutputs {
+  var backedRewardIndexPath: Signal<IndexPath, Never> { get }


Oh hey could we rename this to scrollToBackedRewardIndexPath just to make it clearer what this output does?

justinswart · 2019-10-24T22:03:31Z

Library/ViewModels/RewardsCollectionViewModel.swift

+      .map(titleForContext)
+
+    self.backedRewardIndexPath = Signal.combineLatest(project, rewards)
+      .takeWhen(self.viewDidLayoutSubviewsProperty.signal.ignoreValues())


Interesting 🤔 I believe it needs a run loop to render the cells. We could probably go ahead with this but for interest sake, if you used viewWillAppear and added .ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler) which is a delay of 0 and just puts the emission on the next run loop, that would also work.

…rter/ios-oss into select-new-reward-polish

justinswart

🚢 🚢 🚢

justinswart added 30 commits October 8, 2019 17:45

Updated validation

3a5e5bd

Fix snapshot tests, add submitButtonHidden

fffa982

Add PledgeAmountSummaryViewController

97f4cd8

Merge branch 'master' into change-payment-method-ui

02f290e

Add updateButtonTapped

02bebe8

Add snapshot test and vm test

82b6642

lol

a4b8ffd

Merge branch 'master' into change-payment-method-ui

e70e2e6

Remove unused var

5cbf40b

Rename signal, fix payment source ID comparison

afa9c9a

Fix tests

e2b3afd

Merge branch 'master' into change-payment-method-ui

1d69e99

Fix tests, simplify hiding of shipping location stackview

c189ebe

Wire up change payment method to mutation for payment source and Appl…

031ebc2

…e Pay

Add updateReward context to PledgeViewModel

b1de7b0

Add tests

7c9f91c

Merge branch 'master' into change-payment-method-ui

5a03263

Merge branch 'change-payment-method-ui' into change-payment-method-mu…

30e77cc

…tation

Merge branch 'change-payment-method-mutation' into select-this-reward…

891f50b

…-instead

Fix line lengths

5bf6de3

Merge branch 'master' into change-payment-method-ui

3a6ae51

Merge branch 'change-payment-method-ui' into change-payment-method-mu…

189adbe

…tation

Remove payment method when backing with Apple Pay and vice versa

5ae6f86

Update tests for apple pay sheet race condition fixes

d68df53

Replace zip with simpler signals

a8092af

Add Stripe token error test

d65823d

Fix snapshots

4c49d3a

Commit the actual snapshots

0ed69e0

Merge branch 'change-payment-method-ui' into change-payment-method-mu…

6d7bf10

…tation

justinswart and others added 6 commits October 16, 2019 14:23

Hide payment methods when selecting a new reward

4445e9b

Added output to change Carousel title according to context

f6a3f26

Fixed and added test for title

c079a94

Added logic to centralize backed reward

d406f97

SwiftFormat

7d6991c

Scollaco requested a review from justinswart October 17, 2019 20:26

Scollaco assigned justinswart Oct 17, 2019

Scollaco changed the title ~~Select new reward polish~~ [NT-361] Select new reward polish Oct 17, 2019

justinswart suggested changes Oct 21, 2019

View reviewed changes

Merge branch 'master' into select-new-reward-polish

fbb661d

# Conflicts: # Library/ViewModels/PledgeAmountViewModelTests.swift # Library/ViewModels/PledgeViewModelTests.swift # Library/ViewModels/RewardsCollectionViewModelTests.swift

justinswart changed the base branch from select-this-reward-instead to master October 22, 2019 00:13

justinswart and others added 3 commits October 21, 2019 17:15

Remove code that crept back in during merge

311f48e

Fixed logic to centralize the backed reward

c84f8ad

Fixed test

88ba19b

Scollaco commented Oct 23, 2019

View reviewed changes

justinswart reviewed Oct 23, 2019

View reviewed changes

Scollaco added 2 commits October 24, 2019 12:56

Merge branch 'master' into select-new-reward-polish

38ac910

Merge branch 'master' into select-new-reward-polish

044fcef

ifbarrera mentioned this pull request Oct 24, 2019

[NT-425] Manage Pledge Refresh #905

Merged

3 tasks

justinswart reviewed Oct 24, 2019

View reviewed changes

Scollaco added 6 commits October 25, 2019 13:59

Merge branch 'master' into select-new-reward-polish

b294416

Renamed output

cd582dd

Merge remote-tracking branch 'oss/master' into select-new-reward-polish

c0cfd26

Updated private func to use sharedFunction

bd02acd

Swiftformat

880355e

Merge branch 'select-new-reward-polish' of https://github.com/kicksta…

d6261bb

…rter/ios-oss into select-new-reward-polish

justinswart approved these changes Oct 25, 2019

View reviewed changes

Scollaco merged commit 47e2808 into master Oct 25, 2019

Scollaco deleted the select-new-reward-polish branch October 25, 2019 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-361] Select new reward polish #899

[NT-361] Select new reward polish #899

Scollaco commented Oct 17, 2019 •

edited

Loading

justinswart Oct 21, 2019

justinswart Oct 21, 2019

justinswart Oct 21, 2019

Scollaco Oct 23, 2019

justinswart Oct 24, 2019

justinswart left a comment

justinswart Oct 23, 2019

justinswart Oct 24, 2019

justinswart Oct 24, 2019

justinswart left a comment

[NT-361] Select new reward polish #899

[NT-361] Select new reward polish #899

Conversation

Scollaco commented Oct 17, 2019 • edited Loading

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

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

justinswart left a 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

justinswart left a comment

Choose a reason for hiding this comment

Scollaco commented Oct 17, 2019 •

edited

Loading