Skip to content

Changes to adopt the named arrival instructions from Directions API #1406

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

Merged
merged 13 commits into from
May 15, 2018

Conversation

vincethecoder
Copy link
Contributor

@vincethecoder vincethecoder commented May 9, 2018

Since Directions API will be providing waypoint arrival instructions, we decided to delete the special case for arrival banners.

Closes #1123

TODO:

3usjt

vfchy

jipe7

vtofy

/cc @1ec5 @bsudekum

@vincethecoder vincethecoder self-assigned this May 9, 2018
@vincethecoder vincethecoder requested review from bsudekum and 1ec5 May 9, 2018 15:21
@vincethecoder vincethecoder added ⚠️ DO NOT MERGE PR should not be merged! and removed ⚠️ DO NOT MERGE PR should not be merged! labels May 9, 2018
@bsudekum
Copy link
Contributor

bsudekum commented May 9, 2018

@vincethecoder I think there maybe a couple more additions needed here

  1. Update the example to show how you can name waypoints. Even something as simple as The user's destination is fine I think. No need to geocode the dropped pin.
  2. Update the dependency MapboxDirections.swift to a version that includes New query parameter, waypoint names added to directions options. mapbox-directions-swift#273

@bsudekum bsudekum added this to the v0.17.1 milestone May 9, 2018
@vincethecoder vincethecoder removed the wip label May 9, 2018
@vincethecoder
Copy link
Contributor Author

vincethecoder commented May 9, 2018

⚠️ This PR should only be merged post new release of the MapboxDirections.swift. ⚠️
/cc @mapbox/navigation-ios

@@ -83,7 +83,7 @@ open class ManeuverView: UIView {
ManeuversStyleKit.drawFork(frame: bounds, resizing: resizing, primaryColor: primaryColor, secondaryColor: secondaryColor)
flip = [.left, .slightLeft, .sharpLeft].contains(direction)
case .takeRoundabout, .turnAtRoundabout, .takeRotary:
ManeuversStyleKit.drawRoundabout(frame: bounds, resizing: resizing, primaryColor: primaryColor, secondaryColor: secondaryColor, roundabout_angle: CGFloat(visualInstruction.primaryInstruction.degrees))
ManeuversStyleKit.drawRoundabout(frame: bounds, resizing: resizing, primaryColor: primaryColor, secondaryColor: secondaryColor, roundabout_angle: CGFloat(visualInstruction.primaryInstruction.finalHeading))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change should be reverted, degrees is specific to roundabouts.

Copy link
Contributor Author

@vincethecoder vincethecoder May 9, 2018

Choose a reason for hiding this comment

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

This is a temporary change, as The current master copy of MapboxDirections.swift is missing degrees. Had to use finalHeading till we publish the changes which has the degrees field.
https://github.com/mapbox/MapboxDirections.swift/blob/master/MapboxDirections/MBVisualInstruction.swift#L38

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wow, indeed it was renamed mapbox/mapbox-directions-swift@03979d6

let coordinates = mapView.convert(tap.location(in: mapView), toCoordinateFrom: mapView)
let waypoint = Waypoint(coordinate: coordinates)
let waypoint = Waypoint(coordinate: coordinates, name: destinationName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to use waypoints.count here to show this can change for a multi-waypoint route?

@@ -154,8 +154,9 @@ class ViewController: UIViewController, MGLMapViewDelegate, CLLocationManagerDel
multipleStopsAction.isEnabled = false
}

let destinationName = "Dropped Pin #\(waypoints.endIndex + 1)" // Note you can change this. This will be used in the top banner
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be used in the top banner

to

This will be used in the top banner when arriving at a destination.

@bsudekum
Copy link
Contributor

bsudekum commented May 9, 2018

@vincethecoder MapboxDirections.swift v0.21.0 has been released. Don't forget to update the appropriate podspec files as well.

@vincethecoder vincethecoder removed the ⚠️ DO NOT MERGE PR should not be merged! label May 10, 2018
@bsudekum
Copy link
Contributor

Don't forget to also update MapboxCoreNavigation.podspec.

@vincethecoder vincethecoder requested a review from bsudekum May 10, 2018 16:24
@bsudekum bsudekum added the ⚠️ DO NOT MERGE PR should not be merged! label May 10, 2018
Copy link
Contributor

@bsudekum bsudekum left a comment

Choose a reason for hiding this comment

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

Looks good to me, let's hold off on merging this until v0.17.0 is out.

@bsudekum bsudekum removed the ⚠️ DO NOT MERGE PR should not be merged! label May 14, 2018
@bsudekum bsudekum merged commit c85302d into master May 15, 2018
@bsudekum bsudekum deleted the navigation/arrival_banners_enhancement branch May 15, 2018 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants