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

Ability to swipe InstructionsBannerView #1750

Merged
merged 9 commits into from
Sep 29, 2018
Merged

Ability to swipe InstructionsBannerView #1750

merged 9 commits into from
Sep 29, 2018

Conversation

nitaliano
Copy link
Contributor

  • Added swipeable property to InstructionsBannerView
  • Added didSwipeInstructionsBanner to InstructionsBannerViewDelegate
  • Deprecated didDragInstructionsBanner for InstructionsBannerViewDelegate
  • Updates RouteMapViewController to implement swipeable banner

@1ec5 @frederoni

@@ -546,6 +556,15 @@ class RouteMapViewController: UIViewController {
return populated(named)
}
}

fileprivate func leg(with step: RouteStep) -> RouteLeg? {
Copy link
Contributor

Choose a reason for hiding this comment

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

leg(containing:)

fileprivate func leg(with step: RouteStep) -> RouteLeg? {
for leg in route.legs {
if leg.steps.contains(step) {
return leg
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more succinctly written as route.legs.first { $0.steps.contains(step) }.

updateCameraAltitude(for: router.routeProgress)

mapView.addArrow(route: router.route,
legIndex: router.routeProgress.legIndex,
stepIndex: router.routeProgress.currentLegProgress.stepIndex + 1)

if currentPreviewInstructionBannerStepIndex != nil {
currentPreviewInstructionBannerStepIndex = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think you can set this unconditionally.

@@ -18,8 +18,14 @@ public protocol InstructionsBannerViewDelegate: class {
/**
Called when the user drags either up or down on the `InstructionsBannerView`.
*/
@available(*, deprecated, message: "Please use didSwipeInstructionsBanner instead.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR causes didDragInstructionsBanner(_:) to no longer get called. Deprecating it would be misleading, because that would imply (especially to Objective-C code, via a warning) that it’s OK to keep using the old method for now. Either mark this method obsoleted or, preferably, conditionally call the old method if it’s implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am calling didDragInstructionsBanner(_:) again

_ = curLegSteps.popLast()

for step in curLegSteps {
steps.append(step)
Copy link
Contributor

Choose a reason for hiding this comment

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

Implement a remainingLegs computed property like this:

return Array(legs.suffix(from: legIndex + 1))

Then this method becomes simply:

return currentLegProgress.remainingSteps + remainingLegs.flatMap { $0.steps }

The one wrinkle is that you might want to merge waypoint arrivals with the subsequent departures. We do that in a few places, but it would be misleading to do that as part of a simply named RouteProgress.remainingSteps method. So the caller could do something like this:

routeProgress.remainingSteps.filter { $0.maneuverType != .depart }

@1ec5 1ec5 added feature New feature request. topic: instructions labels Sep 28, 2018
@1ec5 1ec5 added this to the v0.22.0 milestone Sep 28, 2018
@nitaliano
Copy link
Contributor Author

@1ec5 I made all changes

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

A suggestion for the changelog but otherwise good to go. Thanks!

CHANGELOG.md Outdated
@@ -1,6 +1,9 @@
# Changes to the Mapbox Navigation SDK for iOS

## master
* Added `swipeable` property to `InstructionsBannerView`
* Added `didSwipeInstructionsBanner` to `InstructionsBannerViewDelegate`
* Deprecated `didDragInstructionsBanner` for `InstructionsBannerViewDelegate`
Copy link
Contributor

Choose a reason for hiding this comment

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

Fully qualify these symbols for jazzy autolinking:

  • Added an InstructionsBannerView.swipeable property that allows the user to swipe the banner to the side to preview future steps. The InstructionsBannerViewDelegate.didDragInstructionsBanner(_:) method has been deprecated in favor of InstructionsBannerViewDelegate.didSwipeInstructionsBanner(_:swipeDirection:). (#1750)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request. topic: instructions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants