Add example with MapboxNavigation driving separate UI components #1219
Conversation
|
I think we should
|
danesfeder
force-pushed
the
dan-component-navigation-example
branch
5 times, most recently
from
3b96aef to
455c460
Compare
|
@Guardiola31337 this is also ready for review unless @nitaliano you have anything else to add here |
|
I'm good on my end |
There was a problem hiding this comment.
This looks great so far @danesfeder @nitaliano
I left some minor comments / questions to consider before merging this PR.
Are all the comments in ComponentNavigationActivity really necessary? Have we considered extracting blocks of code into well-named (based on the comments) private methods? This way the overall code will be easier to read/understand and comments will become superfluous so they won't be necessary. In general, we should keep public methods as clean as possible so they read like pseudocode (i.e. no conditionals, fors, etc.).
| Intent intent = new Intent(view.getContext(), samples.get(position).getActivity()); | ||
| startActivity(intent); | ||
| } | ||
| view.setOnClickListener(view1 -> { |
There was a problem hiding this comment.
What about using a different naming here? view1 looks kind of vague to me.
| MapboxMap.OnMapLongClickListener, LocationEngineListener, ProgressChangeListener, | ||
| MilestoneEventListener, OffRouteListener { | ||
|
|
||
| private static final String ACCESS_TOKEN = Mapbox.getAccessToken(); |
There was a problem hiding this comment.
What about removing this constant and use Mapbox.getAccessToken() in the call sites?
Noting that we do that across the rest of the activities (examples) in the test app and even here 👀
| private static final int FIRST = 0; | ||
| private static final int ONE_HUNDRED_MILLISECONDS = 100; | ||
| private static final int BOTTOMSHEET_PADDING_MULTIPLIER = 4; | ||
| private static final int TWO_SECONDS = 2000; |
There was a problem hiding this comment.
What about TWO_SECONDS_IN_MILLISECONDS instead?
| private MapState mapState; | ||
|
|
||
| private enum MapState { | ||
| Info, |
There was a problem hiding this comment.
Enum type's fields are constants so they should be in capital letters.
Because they are constants, the names of an enum type's fields are in uppercase letters.
|
|
||
| // Allow navigationMap clicks now that we have the current Location | ||
| navigationMap.retrieveMap().addOnMapLongClickListener(this); | ||
| showSnackbar("Long press the map to select a destination.", BaseTransientBottomBar.LENGTH_LONG); |
There was a problem hiding this comment.
Minor thing - In order to increase readability, what about replacing magic numbers (literals in general including Strings) with constants with a particular meaning?
Further information: Replace Magic Number with Symbolic Constant.
| locationEngine.addLocationEngineListener(this); | ||
| locationEngine.setFastestInterval(1000); | ||
| locationEngine.activate(); | ||
| showSnackbar("Searching for GPS...", BaseTransientBottomBar.LENGTH_SHORT); |
| if (lastLocation == null) { | ||
| return; | ||
| } | ||
| CameraPosition cameraPosition = buildCameraPositionFrom(lastLocation, 0d); |
There was a problem hiding this comment.
Same comment re: magic numbers
| @NonNull | ||
| private CameraPosition buildCameraPositionFrom(Location location, double bearing) { | ||
| return new CameraPosition.Builder() | ||
| .zoom(12.0) |
There was a problem hiding this comment.
Same comment re: magic numbers
| .zoom(12.0) | ||
| .target(new LatLng(location.getLatitude(), location.getLongitude())) | ||
| .bearing(bearing) | ||
| .tilt(0d) |
| int mapViewHeight = mapView.getHeight(); | ||
| int bottomSheetHeight = (int) resources.getDimension(R.dimen.component_navigation_bottomsheet_height); | ||
| int topPadding = mapViewHeight - (bottomSheetHeight * BOTTOMSHEET_PADDING_MULTIPLIER); | ||
| navigationMap.retrieveMap().setPadding(0, topPadding, 0, 0); |
There was a problem hiding this comment.
Same comment re: magic numbers
danesfeder
force-pushed
the
dan-component-navigation-example
branch
2 times, most recently
from
df27cb1 to
70745a4
Compare
|
@Guardiola31337 this is updated and ready for another round of 👀
I know what you're saying with regard to the comments, but I think it's okay to be overly verbose in these test applications. As developers look to implement the SDK and see these examples, I believe they will make things a little more clear.
FWIW, in addition to the extra comments in the code, most of the |
danesfeder
force-pushed
the
dan-component-navigation-example
branch
2 times, most recently
from
ccecae0 to
836d53c
Compare
There was a problem hiding this comment.
I know what you're saying with regard to the comments, but I think it's okay to be overly verbose in these test applications. As developers look to implement the SDK and see these examples, I believe they will make things a little more clear.
Although, I agree with your point here, I also think that there are other ways to improve documentation in general (which would remove the necessity of adding this kind of comments) e.g. fine grained examples, add sample / usage code in Javadocs, be more descriptive in https://github.com/mapbox/android-docs, etc.
That being said, this is not a blocker here and we can discuss later.
FWIW, in addition to the extra comments in the code, most of the
publicmethods in this example are using extractedprivatemethods that are explaining their purpose (or at least, I tried to name them this way).
That's awesome 🚀
Thanks @danesfeder and @nitaliano this new example showcases perfectly how to build a more custom turn-by-turn navigation app with our UI components 
danesfeder
force-pushed
the
dan-component-navigation-example
branch
from
836d53c to
755c44c
Compare
Adds an example to the test application demonstrating
MapboxNavigationsetup / updating anInstructionView+NavigationMapboxMapCurrently uses default styling for map, instruction view, and route line.
cc @nitaliano @d-prukop @bsudekum