-
Notifications
You must be signed in to change notification settings - Fork 310
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
Inform delegate when canceling through eorvc #1318
Conversation
@@ -459,7 +459,7 @@ func defaultFeedbackHandlers(source: FeedbackSource = .user) -> (send: FeedbackV | |||
endOfRoute.dismissHandler = { [weak self] (stars, comment) in | |||
guard let rating = self?.rating(for: stars) else { return } | |||
self?.routeController.setEndOfRoute(rating: rating, comment: comment) | |||
self?.dismiss(animated: true, completion: nil) | |||
self?.delegate?.mapViewControllerDidCancelNavigation(self!) |
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.
Perhaps we should add a new …didEndNavigation()
delegate method? /cc @JThramer
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.
@frederoni instead of force unwrapping self
, why not safely unwrap at the guard
on line 460
?
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.
@frederoni agreed, It's time. Although what about something like didEndNavigation(cancelled: Bool)
?
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.
The optionally chained self?
and delegate?
is evaluated before self!
so this could arguably be considered a safe unwrap. I preferred this way because we'd still want the feedback and the delegate to trigger rather than getting stuck in the guard.
86d11ab
to
2b16f3d
Compare
d46c87f
to
f7b44f9
Compare
Dismissing a view controller only works if it was presented with a UINavigationViewController. By informing the delegate when the navigation is ended, we dismiss by default but also let the developer pop or replace the view controller by implementing
NavigationViewController.mapViewControllerDidCancelNavigation(_:)
.@JThramer