-
Notifications
You must be signed in to change notification settings - Fork 314
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
Separate custom UI example #301
Conversation
import AVFoundation | ||
import MapboxDirections | ||
|
||
class CustomNavigationUI: UIViewController, MGLMapViewDelegate, AVSpeechSynthesizerDelegate { |
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 class name should end with ViewController
, however, CustomNavigationViewController
sounds like it's a subclass of NavigationViewController
which is misleading.
@bsudekum I've picked this up and will continue work on this tomorrow. |
@bsudekum or @frederoni want to take a 👀 when you have a chance? |
if let upComingStep = routeProgress.currentLegProgress.upComingStep { | ||
// Don't give full instruction with distance if the alert type is high | ||
if alertLevel == .high { | ||
text = upComingStep.instructions |
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.
nit, indent off here and line #74
guard let style = mapView.style else { return } | ||
guard let userRoute = userRoute else { return } | ||
|
||
mapView.addAnnotation(destination) |
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'll also want to clear the map of all annotations before adding a new destination.
} | ||
} | ||
|
||
func speak(_ text: String) { |
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.
Since this is only used in one place, I think we can remove this function and move it up to where speak()
is called.
let lineCasing = MGLLineStyleLayer(identifier: identifier, source: source) | ||
// MARK: - Navigation with multiple waypoints | ||
|
||
func startMultipleWaypoints() { |
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.
Since waypoints is coming through the pipeline in #270, do you think we should hold off on showing developers this method for doing multi-leg routes? It's slightly an antipattern.
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.
Let's put this in for now for customers who are asking about multiple destinations.
This example serves both to show how to handle multiple destinations, but also how to display a custom UI over navigation and how to update the route on the fly.
Once this is in master, can you rebase #270 and update the example with how to implement the same concept with waypoints?
camera.heading = location.course | ||
} | ||
} | ||
navigationViewController.pendingCamera = camera |
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.
Setting the initial camera should be added back
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.
Is there a reason this isn't handled by the SDK by default? NavigationViewController should automatically set the pending camera to the currentLocation || route origin's location and initial bearing.
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.
Yeah this should be possible.
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.
RouteMapViewController.viewWillAppear(_:)
falls back to calling setDefaultCamera(_:)
, which only sets the altitude and pitch. It would be feasible for this method to also set the center coordinate to the route’s initial location and heading. However, it wouldn’t be feasible to set the camera to the user’s current location right off the bat, because the location manager determines the current location asynchronously.
Before this PR lands, we should modify setDefaultCamera(_:)
to account for the route’s origin.
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.
Might want to double-check that the initial camera is reasonable. Otherwise, this looks good.
Examples/Swift/ViewController.swift
Outdated
options.includesSteps = true | ||
options.routeShapeResolution = .full | ||
options.profileIdentifier = .automobileAvoidingTraffic | ||
|
||
_ = Directions.shared.calculate(options) { [weak self] (waypoints, routes, error) in | ||
_ = Directions.shared.calculate(options) { (waypoints, routes, error) in |
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.
Out of curiosity, why has self gone from weak to strong here?
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.
Oh, yeah should [weak self] since Directions.shared is a singleton.
Examples/Swift/ViewController.swift
Outdated
// MARK: - Basic Navigation | ||
|
||
func startBasicNavigation() { | ||
guard let route = self.currentRoute else { return } |
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.
Nit: drop self.
.
camera.heading = location.course | ||
} | ||
} | ||
navigationViewController.pendingCamera = camera |
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.
RouteMapViewController.viewWillAppear(_:)
falls back to calling setDefaultCamera(_:)
, which only sets the altitude and pitch. It would be feasible for this method to also set the center coordinate to the route’s initial location and heading. However, it wouldn’t be feasible to set the camera to the user’s current location right off the bat, because the location manager determines the current location asynchronously.
Before this PR lands, we should modify setDefaultCamera(_:)
to account for the route’s origin.
Examples/Swift/ViewController.swift
Outdated
]) | ||
options.includesSteps = true | ||
options.routeShapeResolution = .full | ||
options.profileIdentifier = .automobileAvoidingTraffic |
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.
Can we get these details from the previous route’s routeOptions
? That way, if the profile happens to be .cycling
, it’ll automatically match when going to the next leg of the trip.
@@ -34,6 +34,14 @@ class RouteMapViewController: UIViewController { | |||
} | |||
return parent.pendingCamera | |||
} | |||
var defaultCamera: MGLMapCamera { |
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 is more like tiltedCamera
or cameraFacingRoad
, since it’s a dynamic value rather than a fixed value.
todo:
/cc @frederoni @1ec5 @ericrwolfe