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

Jump to bottom

Updated NavigationRoute.language to take a Locale #1025

Merged
merged 1 commit into from Jun 19, 2018
Merged

Updated NavigationRoute.language to take a Locale #1025

merged 1 commit into from Jun 19, 2018

Conversation

devotaaabel
Copy link
Contributor

Closes #1024

@devotaaabel devotaaabel self-assigned this Jun 13, 2018
Guardiola31337
Guardiola31337 reviewed Jun 14, 2018
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Now that we use only Locale across the SDK I'm wondering if we should remove inferDeviceLanguage and getNonEmptyLanguage from LocaleUtils. From my experience utility classes become a drawer easily. What do you think?

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRoute.java
@@ -282,8 +282,8 @@ public Builder alternatives(@Nullable Boolean alternatives) {
* Languages</a>
* @since 0.5.0
*/
public Builder language(String language) {
directionsBuilder.language(new Locale(language));
public Builder language(Locale language) {

This comment was marked as resolved.

Sign in to view

@devotaaabel
Copy link
Contributor Author

devotaaabel commented Jun 14, 2018 •
edited

@Guardiola31337 we still use getNonEmptyLanguage in NavigationView and NavigationViewModel when getting the language from the directionsRoute. If we were going to use Locale everywhere, maybe it would make sense for mapbox-java to return the language in RouteOptions and in DirectionsRoute as Locale instead of Strings

cc @danesfeder what do you think? For consistency use Locale for voiceLanguage as well? And request that mapbox-java returns Locale instead of string

@devotaaabel devotaaabel added ⚠️ DO NOT MERGE PR should not be merged! and removed ✓ ready for review labels Jun 14, 2018
@devotaaabel devotaaabel force-pushed the devota-string-to-locale branch 2 times, most recently from 140aea1 to 1b7888d Compare June 18, 2018 14:06
@devotaaabel devotaaabel added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jun 18, 2018
danesfeder
danesfeder approved these changes Jun 19, 2018
View reviewed changes
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.

@devotaaabel this looks good for the given ticket. Let's continue to be aware of aligning the SDK around Locale since that's the decision we are making here cc @Guardiola31337

@danesfeder danesfeder added this to the 0.15.0 milestone Jun 19, 2018
@devotaaabel devotaaabel merged commit fbd9b7e into master Jun 19, 2018
@Guardiola31337 Guardiola31337 deleted the devota-string-to-locale branch June 19, 2018 15:18
@danesfeder danesfeder mentioned this pull request Jun 21, 2018
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guardiola31337 Guardiola31337 Guardiola31337 left review comments

@danesfeder danesfeder danesfeder approved these changes

Assignees

@devotaaabel devotaaabel

Labels
None yet
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@devotaaabel @Guardiola31337 @danesfeder