Skip to content
This repository has been archived by the owner on Dec 21, 2020. It is now read-only.

Add next turn instruction in voice instructions and banner instructions #19

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

boldtrn
Copy link
Member

@boldtrn boldtrn commented Oct 11, 2018

Fixes #13.

This PR make the next turn more visible for the user.

For voice instructions, if the next turn is close to the current turn, we add a , then turn X. For banner instructions we also add the next turn as sub component (related to this issue).

  • We have to experiemnt a bit more with the distance of the next turn at which we should merge the then instruction. Currently this is 100m which seems to work fine in my tests so far.

We should think about how we handle banner instructions. There are a couple of things we can control:

  • When the next turn should be shown (like 200m before the turn)
  • If we should show the next turn instruction at all. (The SDK seems to sometimes add the next turn instruction even if it's not specified in the server response).

@karussell
Copy link
Member

Nice, looks good from a short skimming though

There are a couple of things we can control:

Sure - please apply meaningful defaults and then we can iterate from this :)
BTW: is the title really correct :) ?

@boldtrn boldtrn changed the title Remove alternative route calculation Add next turn instruction in voice instructions and banner instructions Oct 12, 2018
@boldtrn
Copy link
Member Author

boldtrn commented Oct 12, 2018

BTW: is the title really correct :) ?

Sorry about that, I must have missed that. Also the commit is named incorrect. I will correct that when squashing this PR.

Sure - please apply meaningful defaults and then we can iterate from this :)

Ok, I will :)

Add comment and minor rename
@boldtrn
Copy link
Member Author

boldtrn commented Oct 31, 2018

After some more testing I think we can merge the PR in the current state. Most cases where I wished for different instructions are mostly related to #9.

In the following I will call the instruction that is not about the next turn but the turn after the next turn, the "then" instruction, as in "turn right, 'then' turn left".

Other than that I tested to show the "then" banner instruction only shortly before the turn instead of all the time. I personally like to show the "then" banner instruction all the time. I guess that's a matter of taste.

IMHO, the current state is good enough to merge and test the current version. Just keep in mind, we can change the occurrence of the then voice and banner instruction :).

I added a minor improvement and rebased the PR so that the commit message is correct.

@karussell
Copy link
Member

I'm fine with merging then - please go ahead :) !

@boldtrn boldtrn merged commit ef13b9a into master Oct 31, 2018
@boldtrn boldtrn deleted the issue_13 branch October 31, 2018 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge close instructions
2 participants