Skip to content

Added waypoint targets to MapboxDirections request#942

Merged
Guardiola31337 merged 1 commit into
masterfrom
osana-waypoint-target
Dec 18, 2018
Merged

Added waypoint targets to MapboxDirections request#942
Guardiola31337 merged 1 commit into
masterfrom
osana-waypoint-target

Conversation

@osana

@osana osana commented Dec 17, 2018

Copy link
Copy Markdown
Contributor

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

@Guardiola31337 Guardiola31337 left a comment

Copy link
Copy Markdown
Contributor

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?

* @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
Copy Markdown
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
Copy Markdown
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


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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 ;

@Guardiola31337 Guardiola31337 left a comment

Copy link
Copy Markdown
Contributor

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 ✅

@danesfeder danesfeder left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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