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

Increase significantRotateThreshold, fix #4970 #4971

Merged
merged 1 commit into from
Jul 12, 2017

Conversation

dagjomar
Copy link
Contributor

@dagjomar dagjomar commented Jul 11, 2017

Ref #4970

From user testing and feedback I have found that the old threshold value of 4 is too low, causing map to rotate when users are trying to simply pinch zoom the map.

Tested with higher values than 10, but found that 10 is a good value.

Changed the significantRotateThreshold from 4 to 10

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

From user testing and feedback I have found that the old threshold value of 4 is too low, causing map to rotate when users are trying to simply pinch zoom the map.

Tested with higher values than 10, but found that 10 is a good value.
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

This sounds good to me!

Should we also increase the default value of bearingSnap option? It controls how much you have to rotate before it stops snapping back to north. The current value is 7, which also might be a bit low.

@dagjomar
Copy link
Contributor Author

dagjomar commented Jul 12, 2017

Regarding also changing bearingSnap value, it sounds like a good idea, as currently, it is difficult to actually perceive that there exist such a value. (As a user, I am trying to make the map snap north, but unable to make it perceivably happen)

However, that is secondary in importance, and requires some more manual user testing before a value can be confidently set. I can also see some potential caveats with a high value for that, and it begs for it to be an customizable property. Or at least possible to have map.bearingSnap: false - or even 0 to turn it off and other values to set it custom.

@mourner
Copy link
Member

mourner commented Jul 12, 2017

and it begs for it to be an customizable property

It is already — it's a public bearingSnap map option.

@dagjomar
Copy link
Contributor Author

@mourner Wow, that is great! Sorry, I didn't check if it was already an optional value, I just assumed it wasn't because all the values looked like private consts when I glimsed on the code.

Well, anyway, great!

@jfirebaugh jfirebaugh merged commit eaed8fe into mapbox:master Jul 12, 2017
@dagjomar dagjomar deleted the patch-2 branch July 30, 2017 23:32
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.

3 participants