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): Combined stop separated vehicle UI #606

Merged
merged 6 commits into from
Jan 2, 2025

Conversation

EmmaSimon
Copy link
Contributor

@EmmaSimon EmmaSimon commented Dec 21, 2024

Summary

Ticket: Show trip details when there's no predicted vehicle, Handle vehicle "on another trip"

Stacked onto #601

Simulator Screenshot - iPhone 15 - 2024-12-20 at 20 21 24 Simulator Screenshot - iPhone 15 - 2024-12-20 at 20 26 35 Simulator Screenshot - iPhone 15 - 2024-12-20 at 20 25 26

Add the UI for the trip states where a vehicle is finishing a different trip, or where some predictions for the trip exist but there is no assigned vehicle. Also includes explainers for both of these states that you can tap on the trip card to open.

iOS

  • If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?
    • Add temporary machine translations, marked "Needs Review"

android
- [ ] All user-facing strings added to strings resource

Testing

Added unit tests for the updated behavior

@EmmaSimon EmmaSimon requested a review from a team as a code owner December 21, 2024 01:37
@EmmaSimon EmmaSimon requested review from boringcactus and removed request for a team December 21, 2024 01:37
Copy link
Member

@boringcactus boringcactus left a comment

Choose a reason for hiding this comment

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

There's some logic in here that should probably move to the shared code by the time we start working on this page in Android, but it's faster to iterate on changes purely within the iOS side, and it might make more sense to move logic for this page into the shared code all at once after we've had it in people's hands for long enough to be confident it's all correct, so I'm fine if we don't deal with that yet now.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather have these icons set to template mode and rendered in the route's text color than have these always rendered in the same color. I think the only difference would be in Silver Line bus routes, but that seems worth it to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about SL and was thinking that these colors would always be correct, thanks, fixed!

header
VStack(alignment: .leading, spacing: 24) {
explanationHeadline.font(Typography.title2Bold)
explanationImage
Copy link
Member

Choose a reason for hiding this comment

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

Should the explanationImage be marked as accessibilityHidden(true)?

Base automatically changed from es-combined-stop-schedules to main January 2, 2025 15:26
Implements UI for the states when a trip has some predictions but is
missing a vehicle and when the vehicle assigned to the prediction is
finishing a different trip, since they have basically the same UI.
@EmmaSimon EmmaSimon force-pushed the es-combined-prediction-missing-vehicle branch from 3714bea to e03f2d7 Compare January 2, 2025 16:04
@EmmaSimon
Copy link
Contributor Author

There's some logic in here that should probably move to the shared code by the time we start working on this page in Android, but it's faster to iterate on changes purely within the iOS side, and it might make more sense to move logic for this page into the shared code all at once after we've had it in people's hands for long enough to be confident it's all correct, so I'm fine if we don't deal with that yet now.

Yeah that's a good point, I think for now I'll leave it in iOS until the behavior is more settled.

@EmmaSimon EmmaSimon merged commit fea79c2 into main Jan 2, 2025
7 checks passed
@EmmaSimon EmmaSimon deleted the es-combined-prediction-missing-vehicle branch January 2, 2025 19:04
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