-
Notifications
You must be signed in to change notification settings - Fork 319
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
Fixed unit type bug #769
Fixed unit type bug #769
Conversation
@@ -814,15 +814,15 @@ private void updateLandscapeConstraintsTo(int layoutRes) { | |||
* @param model to provide the current steps and unit type | |||
*/ | |||
private void updateInstructionList(InstructionModel model) { | |||
instructionListAdapter.updateSteps(model.getProgress()); | |||
instructionListAdapter.updateSteps(getContext(), model.getProgress(), locale, unitType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the locale
and unitType
moved here so they can be updated on the fly and the list will reflect the latest changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danesfeder yes because otherwise, the locale was being set in the adapter when it was initialized, which was before the locale was actually set in InstructionView
public void updateSteps(RouteProgress routeProgress) { | ||
public void updateSteps(Context context, RouteProgress routeProgress, | ||
Locale locale, @NavigationUnitType.UnitType int unitType) { | ||
if (distanceUtils == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to store locale
or unitType
and then check if they have changed / re-initialize DistanceUtils
here if they have? I believe the formatting elsewhere (like in InstructionView
) can change on the fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danesfeder I agree, we should probably add something like that to InstructionStepResource
too so we're not creating a new DistanceUtils
object everytime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks for the quick turnaround on this 🚀
No description provided.