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

Update Default Zoom Level #655

Merged
merged 5 commits into from
Jan 29, 2018
Merged

Update Default Zoom Level #655

merged 5 commits into from
Jan 29, 2018

Conversation

sarahsnow1
Copy link
Contributor

@sarahsnow1 sarahsnow1 commented Jan 18, 2018

This PR lays the foundation for providing an easy way to customize the camera via a new abstract Camera class which can be set with MapboxNavigation#setCameraEngine. It moves the camera positioning logic from NavigationCamera into a new class SimpleCamera. By default, MapboxNavigation now uses an instance of SimpleCamera to determine how the camera should be position while routing. SimpleCamera's default zoom values have also been updated so that they are consistent with iOS.

Further work can use the properties passed in RouteInformation to determine whether the user is starting the route, in the middle of it, or at their destination.

  • Provide screenshot references
  • Add test coverage for SimpleCamera (verify correct values returned, verify initial values cached)

Closes #602

@@ -45,13 +45,11 @@ dependencies {
// Mapbox Android Services
api (dependenciesList.mapboxServices) {
transitive = true
exclude module: 'mapbox-java-geojson'
exclude module: 'mapbox-java-core'
exclude module: 'mapbox-java-services'
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 it would make sense to re-exclude these modules to prevent duplicate imports. mapboxServices is the 2.2.9 version of our services library, which we are using for just telemetry at the moment. mapboxSdkServices is the 3.0.0 of the library and includes the modules above. Sorry for the confusion on this as we transition these libraries!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops- okay, they are now re-excluded. Thanks for clarifying!

@danesfeder
Copy link
Contributor

@sarahlensing Thanks for the work on this - this will make it a lot easier to drive a dynamic camera along a route. Just a minor comment regarding re-excluding those dependencies.

@ericrwolfe Bumping the zoom by one and keeping the same target distance pushes the puck further into the middle of the map:

updated_zoom

The way we calculate the target distance based on the NavigationView and overall screen height won't work well if we dynamically change the zoom (something we want to do in the future). We really should be placing the camera / puck in reference to the bottom sheet (out of scope for this PR but just noting).

Doubling target distance seemed to work pretty well, testing on a 4 different devices with varying screen densities and sizes. For the sake of the zoom bump, this looks like something we can do for now until we figure out a better way to address the target distance.

double_distance

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.

@sarahlensing this looks good to go on our end. If you want to write those tests prior to merging, please feel free :)

@ericrwolfe I'll address the target distance adjustments in a new PR (most likely the one in regard to a more dynamic camera)

@danesfeder
Copy link
Contributor

@sarahlensing I'm going to go ahead and merge this, as we are looking to build off of it for the rest of the dynamic camera work. I cut a ticket to track the test integration #678

@danesfeder danesfeder merged commit 860f270 into master Jan 29, 2018
@danesfeder danesfeder deleted the sarah-default-zoom branch January 29, 2018 15:16
@danesfeder danesfeder mentioned this pull request Feb 26, 2018
13 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.

2 participants