-
Notifications
You must be signed in to change notification settings - Fork 24
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
iOS Route Overview #330
base: main
Are you sure you want to change the base?
iOS Route Overview #330
Conversation
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.
Here are some things to look at in a first pass before I get too far down the rabbit hole.
) | ||
} | ||
|
||
return MapViewCamera.boundingBox(bounds, edgePadding: UIEdgeInsets(top: 20, left: 100, bottom: 20, right: 100)) |
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 padding values are silly because the implementation in the MapLibre SwiftUI DSL seems to be broken.
if let overviewCamera = navigationState?.routeOverviewCamera { | ||
camera = overviewCamera | ||
} | ||
} : .showRecenter { // TODO: Third case when not navigating! |
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.
TBD the best way to express this logic given the limits of view code; probably in an extension / helper function.
/// Determines which camera control is shown in the navigation inner grid view. | ||
public enum CameraControlState { | ||
/// Don't show any camera control. | ||
case hidden | ||
|
||
/// Shows the recenter button. | ||
/// | ||
/// The action is responsible for resetting the camera | ||
/// to a state that follows the user. | ||
case showRecenter(() -> Void) | ||
|
||
/// Shows the route overview button. | ||
/// | ||
/// The action is responsible for transitioning the camera to an overview of the route. | ||
case showRouteOverview(() -> Void) | ||
} |
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 the first significant design decision. I think that an enum makes the camera control states significantly cleaner to express. We can add more later if necessary, but the common ones are here.
// TODO: Extract to a separate view? | ||
switch cameraControlState { | ||
case .hidden: | ||
// Nothing | ||
EmptyView() | ||
case let .showRecenter(onCenter): | ||
NavigationUIButton(action: onCenter) { | ||
Image(systemName: "location.north.fill") | ||
.resizable() | ||
.aspectRatio(contentMode: .fit) | ||
.frame(width: 18, height: 18) | ||
} | ||
.shadow(radius: 8) | ||
case let .showRouteOverview(onRouteOverview): | ||
NavigationUIButton(action: onRouteOverview) { | ||
Image(systemName: "point.bottomleft.forward.to.point.topright.scurvepath") | ||
.resizable() | ||
.aspectRatio(contentMode: .fit) | ||
.frame(width: 18, height: 18) | ||
} | ||
.shadow(radius: 8) | ||
} |
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 important thing to note here is that we have moved the recenter button to be in the top corner. The logic behind that is that we probably only ever care about having one camera button visible at a time in my opinion (differing opinions welcome :D). So the camera button slot will either be a recenter button, a route overview button, or hidden, depending on state driven higher up.
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.
Discussed; we will move the recenter button to the bottom left corner to keep with convention.
} bottomLeading: { | ||
bottomLeading?() |
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.
Undo this change; recenter will stay in lower left corner.
// TODO: Extract to a separate view? | ||
switch cameraControlState { | ||
case .hidden: | ||
// Nothing | ||
EmptyView() | ||
case let .showRecenter(onCenter): | ||
NavigationUIButton(action: onCenter) { | ||
Image(systemName: "location.north.fill") | ||
.resizable() | ||
.aspectRatio(contentMode: .fit) | ||
.frame(width: 18, height: 18) | ||
} | ||
.shadow(radius: 8) | ||
case let .showRouteOverview(onRouteOverview): | ||
NavigationUIButton(action: onRouteOverview) { | ||
Image(systemName: "point.bottomleft.forward.to.point.topright.scurvepath") | ||
.resizable() | ||
.aspectRatio(contentMode: .fit) | ||
.frame(width: 18, height: 18) | ||
} | ||
.shadow(radius: 8) | ||
} |
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.
Discussed; we will move the recenter button to the bottom left corner to keep with convention.
Opening this as a draft PR because there are still a few problems with it, but I want feedback on the overall approach. I'll leave comments to guide the first review.
Ignore the terrible quality of simulator animated GIFs... it is not really that choppy, but it DOES take too long to get back to the user's real position because of intermediate camera updates, which is a problem we'll need to fix.