-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
New backer push navigate to activity #222
Conversation
…essageTreads view
…ickstarter/ios-oss into creator_message_notification_1
…tivity # Conflicts: # Library/ViewModels/DashboardViewModel.swift
…tivity # Conflicts: # Library/ViewModels/DashboardViewModel.swift
…tivity # Conflicts: # Library/ViewModels/DashboardViewModel.swift
# Conflicts: # Kickstarter-iOS/AppDelegate.swift # Kickstarter-iOS/ViewModels/AppDelegateViewModel.swift # Kickstarter-iOS/Views/Controllers/DashboardViewController.swift # Kickstarter-iOS/Views/Controllers/RootTabBarViewController.swift # Library/Navigation.swift # Library/ViewModels/DashboardViewModel.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Boris! Couple small things.
Kickstarter-iOS/AppDelegate.swift
Outdated
private func goToProjectActivities(_ projectId: Param) { | ||
self.rootTabBarController? | ||
.swithchToProjectActivities(projectId: projectId) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this new line.
Kickstarter-iOS/AppDelegate.swift
Outdated
@@ -319,6 +325,12 @@ internal final class AppDelegate: UIResponder, UIApplicationDelegate { | |||
.switchToCreatorMessageThread(projectId: projectId, messageThread: messageThread) | |||
} | |||
|
|||
private func goToProjectActivities(_ projectId: Param) { | |||
self.rootTabBarController? | |||
.swithchToProjectActivities(projectId: projectId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fit on one line and there's a spelling error swithch
-> switch.
public func swithchToProjectActivities(projectId: Param) { | ||
self.switchToDashboard(project: nil) | ||
|
||
guard let dashboardNav = self.selectedViewController as? UINavigationController, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we do double let
s in a guard
we like to nest them for readability:
guard
let dashboardNav = self.selectedViewController as? UINavigationController,
let dashboardVC = dashboardNav.viewControllers.first as? DashboardViewController
else {
...
}
navigateToActivitiesReceived.producer | ||
.skipNil() | ||
.filter { $0 == .id(project.id) } | ||
.map { _ in project } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if this would read better as .mapConst(project)
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me personally .map { _ in project } feels more explicit.
@@ -772,7 +774,7 @@ final class AppDelegateViewModelTests: TestCase { | |||
launchOptions: [UIApplicationLaunchOptionsKey.remoteNotification: backingForCreatorPushData] | |||
) | |||
|
|||
self.goToDashboard.assertValues([param]) | |||
self.goToProjectActivities.assertValues([param]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to the test for goToDashboard
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are going to ProjectActivities instead of going to the Dashboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm down, pinging @stephencelis too.
What
Clicking on the new backer notification opens activities view for the corresponding project
How
Expanded on the concept used for "Creator message push" functionality:
Added Project activity navigation node
Added navigateToActivities management signals and producers in the DashboardViewModel
See 👀
Trello