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

Feat/ios speed limit view #119

Merged
merged 23 commits into from
Jul 2, 2024
Merged

Feat/ios speed limit view #119

merged 23 commits into from
Jul 2, 2024

Conversation

Archdoog
Copy link
Collaborator

No description provided.

@Archdoog Archdoog marked this pull request as ready for review June 20, 2024 03:34
@Archdoog Archdoog requested a review from ianthetechie June 20, 2024 03:34
var speedWithUnitsFormatter: MeasurementFormatter { get }
}

public struct DefaultFerrostarTheme: FerrostarTheme {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sold on a "theme" also including locale settings. Those seem like separate concepts.

apple/Sources/FerrostarSwiftUI/Views/ArrivalView.swift Outdated Show resolved Hide resolved
Comment on lines 21 to 34
switch locale.identifier {
case "en_US":
USSpeedLimitView(
speedLimit: speedLimit,
valueFormatter: valueFormatter,
unitFormatter: unitFormatter
)
default:
ROWSpeedLimitView(
speedLimit: speedLimit,
valueFormatter: valueFormatter,
unitFormatter: unitFormatter
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this is the best way to go about deciding which speed limit to show. For one, the US isn't the only country to use mph for speed limits.

Second, I'm pretty convinced that locale is not the correct way to set speed display preference.

I think we can punt on the specifics of this for the moment and:

  1. Have it be a pair of preferences for the moment (maybe bundled into the theme / display units / formatter stuff??). It's really a pair of preferences: do you want the US/MUTCD style or Vienna Convention style that's used almost everywhere else. If I were designing this, I'd make it vary on geography, but that's way out of scope for now ;)
  2. Move the logic at the use site (get rid of this view wrapper until we have a better idea of how users want to determine it automatically).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Consider how this could become a SpeedLimitProvider (e.g. to take locale, manual inputs, or a route admins list).

Add comment that this needs to improve in the future. & Make it easy to override.

@ianthetechie
Copy link
Contributor

I also noticed that several of the tests are (now?) nondeterministic.

  1. Dark mode probably needs to be explicitly set for snapshot tests.
  2. Arrival view tests almost all fail on my machine; snapshots expect feet; my laptop uses meters.
  3. Default formatter tests make the same error since they don't explicitly set certain locale details in testDistanceFormatter and testEstimatedArrivalFormatter.

Accepting suggestions

Co-authored-by: Ian Wagner <[email protected]>
@Archdoog
Copy link
Collaborator Author

  1. Add environment settings like dark mode/light mode to tests. Tests need explicit formatters (time zones, etc), environment dark mode & environment locale (units).
  2. Add a readme discussion that the simulator needs to exactly match.

apple/Sources/FerrostarSwiftUI/Theme/FerrostarTheme.swift Outdated Show resolved Hide resolved
Comment on lines 21 to 34
switch locale.identifier {
case "en_US":
USSpeedLimitView(
speedLimit: speedLimit,
valueFormatter: valueFormatter,
unitFormatter: unitFormatter
)
default:
ROWSpeedLimitView(
speedLimit: speedLimit,
valueFormatter: valueFormatter,
unitFormatter: unitFormatter
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Consider how this could become a SpeedLimitProvider (e.g. to take locale, manual inputs, or a route admins list).

Add comment that this needs to improve in the future. & Make it easy to override.

@@ -54,20 +54,21 @@ public struct USStyleSpeedLimitView: View {
.background(Color.white)
.frame(width: 52, height: 76)
.cornerRadius(4)
.colorScheme(.light)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh interesting... Is this to fix the text color (which I guess might default to white in dark mode)? If so, maybe we can just set the text color explicitly?

Copy link
Collaborator Author

@Archdoog Archdoog Jul 2, 2024

Choose a reason for hiding this comment

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

This fixes the speed limit sign colors to light mode. Since it's something we don't want to invert (basically maintaining the white background and black text regardless of the device's color mode).

Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving a comment for posterity because this was EXTREMELY interesting... Apparently named colors (ex: .green) may be aware of light/dark mode, and resolve to different hex values! That's both unexpected but also kinda cool.

apple/Sources/UniFFI/ferrostar.swift Outdated Show resolved Hide resolved
@ianthetechie ianthetechie merged commit cf767e1 into main Jul 2, 2024
11 checks passed
@ianthetechie ianthetechie deleted the feat/ios-speed-limit-view branch July 2, 2024 04:02
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.

3 participants