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

Added waypoint targets to MapboxDirections request #942

Merged
merged 1 commit into from
Dec 18, 2018

Conversation

osana
Copy link
Contributor

@osana osana commented Dec 17, 2018

closes #939

@osana osana force-pushed the osana-waypoint-target branch 2 times, most recently from 2be226c to c01a7a0 Compare December 18, 2018 07:51
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix here, this is looking great @osana 👍

I left some minor comments.

@danpaz confirmed that the first coordinate is ignored (for now) so accepts: any coordinate, empty (;) or null) but mentioned that in the future the first waypoint_target could provide guidance to the starting point of the route. Should we document / add a comment in Javadoc clarifying this?

@@ -47,6 +47,7 @@
* @param exclude exclude tolls, motorways or more along your route
* @param approaches which side of the road to approach a waypoint
* @param waypointNames custom names for waypoints used for the arrival instruction.
* @param waypointTargets list of coordinate pairs for drop-off locations .
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT - Typo extra space before the .

}

/**
* Converts array of Points with waypoint_names values
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT - Typo *waypoint_names should be waypoint_targets

@@ -725,6 +755,23 @@ public Builder addWaypointNames(@Nullable String... waypointNames) {

abstract Builder waypointNames(@Nullable String waypointNames);

/**
* A list of coordinate points used to specify drop-off locations
* that are distinct from the locations specified in coordinates.
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT - Typo extra space in *in coordinates.

/**
* A list of coordinate points used to specify drop-off locations
* that are distinct from the locations specified in coordinates.
* The number of waypoint targets must be the same as the number of coordinates,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here ☝️

* Must be used with steps=true .
* @param waypointTargets list of coordinate points for drop-off locations
* @return this builder for chaining options together
* @since 4.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - NIT *4.2.0 should be 4.3.0

* A semicolon-separated list of coordinate pairs used to specify drop-off
* locations that are distinct from the locations specified in coordinates.
* If this parameter is provided, the Directions API will compute the side of the street,
* left or right, for each target based on the waypoint_targets and the driving direction.
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT - Typo extra space in *on the waypoint_targets

* locations that are distinct from the locations specified in coordinates.
* If this parameter is provided, the Directions API will compute the side of the street,
* left or right, for each target based on the waypoint_targets and the driving direction.
* The maneuver.modifier , banner and voice instructions will be updated with the computed
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here ☝️ *The maneuver.modifier , and list with the ;

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

@osana any of the comments above are blocking the PR ✅

Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

Thanks for the quick work on this @osana - I have no feedback other than what @Guardiola31337 has already pointed out. 🚢

@Guardiola31337
Copy link
Contributor

I went ahead and fix above comments myself so we can 🚀

I hope that was ok with you @osana 🙏

@Guardiola31337 Guardiola31337 merged commit f21558a into master Dec 18, 2018
@Guardiola31337 Guardiola31337 deleted the osana-waypoint-target branch December 18, 2018 18:47
@osana osana mentioned this pull request Dec 18, 2018
14 tasks
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.

Add waypoint targets to MapboxDirections
3 participants