Skip to content

Conversation

@tedi4t
Copy link
Contributor

@tedi4t tedi4t commented Jul 24, 2025

No description provided.

web/index.html Outdated
valhallaEndpointUrl="https://api.stadiamaps.com/route/v1"
styleUrl="https://tiles.stadiamaps.com/styles/outdoors.json"
profile="bicycle"
system="imperial"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the display at the bottom updates, but the navigation banners are still metric ;)
image

@tedi4t tedi4t requested a review from ianthetechie August 6, 2025 20:18
@ianthetechie
Copy link
Contributor

Sorry for the delay getting back to this @tedi4t! I was traveling for the past week or so and this got buried.

Another contributor had been working on further splitting the responsibilities between the core and the map widget in #6622, which created a few conflicts. I think I've resolved all of the conflicts now.

The only outstanding issue I have is that it seems to show decimal places for the "smaller" units. While it's quite reasonable to display something like 0.9 mi or 1.2km, I don't think anyone cares about fractional meters, feet, or yards ;) GPS isn't even that accurate anyways.

image

Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

The latest changes address some of the issues. For example, feet are no longer shown with a decimal value. However, there are a few problems outstanding.

  1. There is a very long stretch where "1 mi" is displayed. For very long distances, yes, we should hide the precision. But typically between 1 and 10 miles (or kilometers), you'd want a slightly more granular display (1 - 2 digits?).
image
  1. The thresholds do not seem to be aligned between when the library switches to feet, and when we do the cutoff here. For example, at this point in the route, there's a significant period of time where we display "0 mi" which isn't very helpful. These should be aligned. Perhaps some of this code belongs in a PR in the formatters library instead, so we can have a holistic view of which unit we're showing and make decisions based on that? (We should never show fractional feet, yards, or meters, but we should SOMETIMES show fractional miles and kilometers).
image

@tedi4t tedi4t force-pushed the feature/new-distance-system branch from 0a15c28 to 72ad5f3 Compare September 1, 2025 18:06
@tedi4t
Copy link
Contributor Author

tedi4t commented Sep 1, 2025

Hello,
@ianthetechie, could you explain how you reproduced it, please? I just tried, but everything looks fine to me based on the test cases you described above. I’m also attaching screenshots. If you think it’s a small change and easier for you to fix directly, I don’t mind.
Screenshot 2025-09-01 at 21 26 49
Screenshot 2025-09-01 at 21 26 21

@tedi4t tedi4t requested a review from ianthetechie September 1, 2025 21:23
@ianthetechie
Copy link
Contributor

Hmm... I'm using the PR branch with no changes locally. I just click "Simulate navigation" and for the longest time it just shows "1 mi" (and even eventually "0 mi" for a short bit) with no further detail. Then it eventually switches to feet.

These should be aligned. Perhaps some of this code belongs in a PR in the formatters library instead, so we can have a holistic view of which unit we're showing and make decisions based on that? (We should never show fractional feet, yards, or meters, but we should SOMETIMES show fractional miles and kilometers).

My point here is that right now there's code within the formatters library which determines the distance at which we switch between, say, miles and feet, or km and meters. This is problematic for a few reasons.

The user of the library code has no idea where the switch (e.g. miles -> feet) happens. Since all we pass is a maximum number of fraction digits, and we don't know the switch-over point, it's essentially impossible to guarantee it behaves as desired. Looking at the code over there, it looks sane enough for metric, but doesn't really make any sense for either imperial setting.

I think we can just hard code this in the formatters library since it's pretty narrow in purpose. Here's the reference logic from Android:

. It looks like you've ported some of the logic, but not all, and having to set an explicit number of (max) fraction digits when calling it is causing the issues.

@tedi4t
Copy link
Contributor Author

tedi4t commented Sep 14, 2025

@ianthetechie
Thanks for the detailed feedback! I understand the issues now. To confirm the required changes:

1. Remove maxDecimalPlaces property from components

I'll remove the maxDecimalPlaces property from:

  • ferrostar-map.ts
  • instructions-view.ts
  • trip-progress-view.ts

And update all formatDistance() calls to only pass (distance, system) parameters.

2. Move formatting logic to the formatters library

I'll update the @maptimy/platform-formatters library to:

  • Handle unit switching internally based on distance thresholds (similar to the Android implementation)
  • Apply appropriate decimal precision per unit:
    • Meters, feet, yards: No decimals
    • Kilometers: Show decimals when < 10 km
    • Miles: Show decimals when < 10 mi
  • Use proper switching points between units (e.g., mi → ft, km → m)

3. Simplify the API

The new API will be:

formatDistance(distance, system)  // No maxDecimalPlaces parameter

@ianthetechie
Copy link
Contributor

Yeah, I think that sounds like a good plan @tedi4t. Thanks!

@tedi4t
Copy link
Contributor Author

tedi4t commented Sep 23, 2025

@ianthetechie as discussed, created new PR: maptimy/platform-formatters#6

@ianthetechie
Copy link
Contributor

Leaving this open for the moment, but see the comment on maptimy/platform-formatters#6. We'll try to get this sorted soon and then update this PR (or close it and make a new one?) to use that accordingly.

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