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-681] CTA button reload fix #996

Merged
merged 12 commits into from
Jan 8, 2020
Merged

Conversation

Scollaco
Copy link
Contributor

📲 What

  • Changes the behaviour of the CTA button on which reloaded everytime the Project Page appeared.

🤔 Why

  • While is important to always show the latest project's data, having the button reloading on viewWillAppear ended up being overkill and having the button "flashing" all the time wasn't a great UX.

🛠 How

  • Creating a notification projectBacked that will send a message to the Project Page once the user taps the X button on the Thanks Page.
  • The function ManagePledgeViewControllerDelegate now sends messages to its observers once a pledge is cancelled or updated (before it was being used only for cancelling).
  • Added and updated tests

✅ Acceptance criteria

The CTA button should load when:

  • Project Page is presented (viewDidLoad)
  • A pledge is updated (After closing the PledgeViewController)
  • A pledge is cancelled
  • A project is backed

The CTA button should not load when:

  • Returning from screens such as Creator's bio, Campaign page, Comments and Updates

GIF

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Cool! Had one or two suggestions.

NotificationCenter.default
.addObserver(
self,
selector: #selector(self.didBackProject),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace self with ProjectPamphletViewController.

self.notifyDelegateShouldDismissAndShowSuccessBannerWithMessage
= self.cancelPledgeDidFinishWithMessageProperty.signal.skipNil()
self.notifyDelegateManagePledgeViewControllerFinishedWithMessage
= Signal.merge(
Copy link
Contributor

Choose a reason for hiding this comment

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

This can fit on the previous line.

@@ -31,7 +31,7 @@ public protocol ThanksViewModelOutputs {
var backedProjectText: Signal<NSAttributedString, Never> { get }

/// Emits when view controller should dismiss
var dismissToRootViewController: Signal<(), Never> { get }
var dismissToRootViewController: Signal<Notification, Never> { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to dismissToRootViewControllerAndPostNotification?

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

🚢 🚢

@Scollaco Scollaco merged commit 0ecef67 into master Jan 8, 2020
@Scollaco Scollaco deleted the NT-681-cta-button-reload-fix branch January 8, 2020 15:36
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