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

Separating the functionality of the previous distance formatter to ma… #5182

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

cafesilencio
Copy link
Contributor

@cafesilencio cafesilencio commented Nov 30, 2021

Description

Addresses what is described in #5065 by refactoring the formatter related code so that the rounding calculations are decoupled from the formatting of the text used in the display. Backwards compatibility is maintained. In addition the new implementation should be usable by the Android Auto implementation which is one of the goals.

Changelog

<changelog>Added `MapboxDistanceUtil` which moves some of the implementation from `MapboxDistanceFormatter` so that the calculation of the distance rounding and the accompanying text is separated from the SpannableString construction done in  `MapboxDistanceFormatter`.</changelog>

Screenshots or Gifs

@cafesilencio cafesilencio added the skip changelog Should not be added into version changelog. label Nov 30, 2021
@cafesilencio cafesilencio requested a review from a team as a code owner November 30, 2021 22:51
@cafesilencio cafesilencio force-pushed the sb-5605-distance-formatter branch 2 times, most recently from 519d83e to 3895d86 Compare November 30, 2021 23:39
@Guardiola31337
Copy link
Contributor

Code wise this is looking good.

I have a question. From #5065

  • Update or expose a distance formatter that can emit a complex object consisting of:
    class Distance( val distance: Double, val unit: String)
    

I saw we opted to expose it through the new MapboxDistanceUtil instead of the DistanceFormatter. @abhishek1508 is that sufficient / will cover the use cases you had in mind?

@cafesilencio
Copy link
Contributor Author

Part of my reasoning was to maintain backward compatibility and avoid breaking semver. The new implementation will support the Nav. SDK as well as the Android Auto implementation.

Copy link
Contributor

@abhishek1508 abhishek1508 left a comment

Choose a reason for hiding this comment

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

LGTM. Let's add a changelog since the PR introduces new API using MapboxDistanceUtil.

/cc @cafesilencio

* @param distanceSuffix a suffix that goes with the string value of the distance like 'km' for kilometers, 'ft' for foot/feet, 'mi' for miles etc. depending on the Locale and the UnitType
* @param unitType indicates if the values represented are metric or imperial
*/
data class FormattedDistanceData(
Copy link
Contributor

Choose a reason for hiding this comment

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

should be internal constructor? if so need to remove data and override quals/toString/hashCode

@cafesilencio cafesilencio removed the skip changelog Should not be added into version changelog. label Dec 14, 2021
@cafesilencio cafesilencio enabled auto-merge (squash) December 14, 2021 21:02
…ke it more usable by components that need a less opinionated formatter.
@cafesilencio cafesilencio merged commit f5d8602 into main Dec 15, 2021
@cafesilencio cafesilencio deleted the sb-5605-distance-formatter branch December 15, 2021 00:01
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.

None yet

4 participants