feat: show filtered departures in stop details #124
Conversation
| Button(action: { | ||
| filter = .init(routeId: patternsByHeadsign.route.id, directionId: patternsByHeadsign.directionId()) | ||
| }, label: { | ||
| NearbyStopRoutePatternView( |
There was a problem hiding this comment.
suggestion (non-blocking): Since this is no longer exclusive to the nearby view, it might be worth renaming this to something more general / descriptive, though that could be a separate follow-up.
There was a problem hiding this comment.
I remember wanting to do that but not being able to think of a perfect component name. Since it's a single row designed to be contained in a trip card, and it may contain one trip or multiple trips but it will always have a headsign, it seems like HeadsignRowView is good enough of a name for it.
| let tripsFormatted: [(PatternsByHeadsign, PatternsByHeadsign.Format)] = trips.map { ($0, $0.format(now: now)) } | ||
| tripData = tripsFormatted.filter { | ||
| let (patternsByHeadsign, formatted) = $0 | ||
| let trip: UpcomingTrip? = patternsByHeadsign.upcomingTrips?.first |
There was a problem hiding this comment.
question: Is splitPerTrip needed? I found it a bit confusing on initial read that trips is a list of PatternsByHeadsign rather than a list of trips, and that by using splitByTrip there is only ever 1 trip in the list of upcomingTrips.
Would it be viable to keep the list of UpcomingTrips grouped by headsign and do a nested ForEach over the list of patternsByHeadsign and the list of upcomingTrips for each headsign? That way when we see PatternsByHeadsign used we can assume it is representing the same data as when it is used in other cases - a list of all trips for a headsign.
There was a problem hiding this comment.
A nested ForEach would not work, because we need the individual trips to be sorted by time even if that means interleaving the headsigns together. We need to be calling PatternsByHeadsign.format() because that's where the logic that hides subway schedules is (since the UpcomingTrip doesn't know its Route directly). However, if we want a more coherent abstraction boundary, we can change splitPerTrip(): List<PatternsByHeadsign> to allUpcomingTrips(): List<UpcomingTrip> and then reconstruct the PatternsByHeadsign as part of the presentation layer instead.
There was a problem hiding this comment.
Ah I hadn't considered the interleaving piece!
| _ = self.popLast() | ||
| self.append(.stopDetails(stop, newValue)) |
There was a problem hiding this comment.
question (non-blocking): Am I reading this correctly that there is only ever 1 item in the navigation stack?
There was a problem hiding this comment.
This code does not make that assumption, although since the only links in the app go from nearby transit to stop details, there is currently no way to have multiple entries in the navigation stack.
| let selectedData = departures.routes.first(where: { $0.route.id == filter.routeId })! | ||
| StopDetailsFilteredRouteView(patternsByStop: selectedData, now: now, filter: $filter) |
There was a problem hiding this comment.
question (non-blocking): the departures are filtered by route here, but by direction within StopDetailsFilteredRouteView. I wonder if for consistency purposes it would make sense to filter by both route + direction either fully inside StopDetailsFilteredRouteView or fully outside of it?
There was a problem hiding this comment.
That makes sense, yeah.
| XCTAssertEqual(stack, []) | ||
| } | ||
|
|
||
| func testLastFilterShallow() throws { |
There was a problem hiding this comment.
nitpick(non-blocking): I'm not picking up what is Shallow vs Deep between these two tests, maybe some additional description would help?
There was a problem hiding this comment.
It's the navigation stack that's shallow when there's no previous history and deep when there's previous history. I've added more pages to the deep test to clarify what sets that test apart from the shallow test.
| @@ -0,0 +1,75 @@ | |||
| // | |||
There was a problem hiding this comment.
praise: Great work unit testing these pieces!
boringcactus
force-pushed
the
mth-stop-direction-filter
branch
from
bf7fa6c to
66582bc
Compare
| tripId = trip.trip.id | ||
| headsign = trip.trip.headsign | ||
| formatted = PatternsByHeadsign( | ||
| route: route, headsign: headsign, patterns: [], upcomingTrips: [trip], alertsHere: nil |
There was a problem hiding this comment.
question (non-blocking): does losing alertsHere data limit how what we can render in alerts scenarios?
There was a problem hiding this comment.
No, because rendering the alert once for each upcoming trip would be incorrect; it looks like the spec for handling alerts in stop details hasn't been written yet, but whatever we settle on will be outside of the individual trips.
| let tripsFormatted: [(PatternsByHeadsign, PatternsByHeadsign.Format)] = trips.map { ($0, $0.format(now: now)) } | ||
| tripData = tripsFormatted.filter { | ||
| let (patternsByHeadsign, formatted) = $0 | ||
| let trip: UpcomingTrip? = patternsByHeadsign.upcomingTrips?.first |
There was a problem hiding this comment.
Ah I hadn't considered the interleaving piece!
Summary
Ticket: Show detailed departures for a route + direction
Implements the full list of upcoming departures for a route and direction filter in the stop details page.
Stacked on #122.
Testing
Manually verified continuing functionality and added new unit tests.