-
Notifications
You must be signed in to change notification settings - Fork 6
feat(iOS): Nearby transit direction grouping UX #893
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
Conversation
Part of this was removing an unnecessary NearbyTransitState binding
11fafb1
to
37af9f7
Compare
3f58363
to
b15e28e
Compare
Also remove a test that was only failing in CI, with a more basic replacement testShowsBranchingHeadsigns in RouteCardDeparturesTests.
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.
Nice work! Just a few small Qs
@@ -42,7 +42,8 @@ sealed class LeafFormat { | |||
*/ | |||
val route: Route?, | |||
val headsign: String, | |||
val format: UpcomingFormat | |||
val format: UpcomingFormat, | |||
val id: String = "$headsign-$format" |
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.
question (non-blocking): Is this required to be unique? I think it might not be in rare cases like bus bunching, where you could have two buses with the same headsign both arriving "Now"
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.
Ah, hmm, good point. This was really just to make the ForEach
happy, so it would just give one of those "the ID X occurs multiple times within the collection, this will give undefined results" errors, which isn't ideal but generally doesn't seem to cause major problems. Maybe I could tack on a UUID?
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.
Added it!
Summary
Ticket: Group by direction | Barebones Nearby Transit UX
Group by direction in nearby.
iOS
- [ ] If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?- [ ] Add temporary machine translations, marked "Needs Review"Testing
NearbyTransitViewLegacyTests
NearbyTransitViewTests
replaces each of the legacy tests with new ones that use direction grouping, except fortestLineGrouping
, which was failing only in CI for unclear reasons, it was replaced withRouteCardDeparturesTests.testShowsBranchingHeadsigns
RouteCardTests
,RouteCardStopHeaderTests
, andRouteCardDeparturesTests
.