feat: Combine existing trip details into new combined stop page #551
Conversation
EmmaSimon
force-pushed
the
es-combined-stop-details-ux
branch
from
02bc84d
to
51bf089
Compare
EmmaSimon
changed the title
EmmaSimon
force-pushed
the
es-combined-stop-details-ux
branch
from
51bf089
to
46e57a0
Compare
| @@ -160,7 +169,7 @@ class ViewportProvider: ObservableObject { | |||
| center: CLLocationCoordinate2D, | |||
| inView: [CLLocationCoordinate2D], | |||
| // Insets with different horizontal/vertical values will result in the center point being off center | |||
| padding: EdgeInsets = .init(top: 75, leading: 50, bottom: 75, trailing: 50) | |||
| padding: EdgeInsets = Defaults.overviewPadding | |||
iosApp/iosApp/Pages/LegacyStopDetails/LegacyStopDetailsPage.swift
Outdated
Show resolved
Hide resolved
| @@ -103,7 +103,7 @@ struct LegacyStopDetailsView: View { | |||
| now: now.toKotlinInstant(), | |||
| filter: filter, | |||
| setFilter: setFilter, | |||
| pushNavEntry: nearbyVM.pushNavEntry, | |||
| pushNavEntry: { entry in nearbyVM.pushNavEntry(entry) }, | |||
There was a problem hiding this comment.
question (non-blocking): is this needed?
There was a problem hiding this comment.
Yeah I didn't love this, it's required because I changed the pushNavEntry signature to take a second optional argument when called from the map, but all these places expect a signature with only a single argument, so they won't compile unless we do this. It can be changed back once we get rid of the feature flag and can directly set the lock boolean when calling the vehicle nav from the map.
| @@ -64,6 +64,7 @@ extension HomeMapView { | |||
| joinVehiclesChannel(routeId: routeId, directionId: directionId) | |||
| } else { | |||
| leaveVehiclesChannel() | |||
| vehiclesData = [] | |||
There was a problem hiding this comment.
question(non-blocking): why move this out of leaveVehiclesChannel?
There was a problem hiding this comment.
Because the vehicles channel was being disconnected and reconnected every time you selected another departure, which was leading to all of the vehicles flickering in and out every time. It would probably make sense to instead not disconnect and reconnect, that can probably happen in a follow up.
| viewportProvider.followVehicle(vehicle: nextVehicle, target: nearbyVM.getTargetStop(global: globalData)) | ||
| } else { | ||
| if nearbyVM.combinedStopAndTrip, let stop = nearbyVM.getTargetStop(global: globalData) { | ||
| viewportProvider.vehicleOverview(vehicle: nextVehicle, stop: stop) |
There was a problem hiding this comment.
question (non-blocking): Should this follow the vehicle as well?
There was a problem hiding this comment.
Asked in figma and was told that we just want to show both on selection, then keep the map static.
| @@ -118,21 +118,22 @@ struct NearbyTransitView: View { | |||
| ScrollView { | |||
| LazyVStack(spacing: 0) { | |||
| ForEach(transit, id: \.id) { nearbyTransit in | |||
| switch onEnum(of: nearbyTransit) { | |||
| let nearby: StopsAssociated = nearbyTransit | |||
There was a problem hiding this comment.
question (non-blocking): could nearbyTransit just be renamed nearby at the top-level?
There was a problem hiding this comment.
This was because I had some unrelated compilation error within the case statement (it was the pushNavEntry arg thing I mentioned above), and instead of giving a useful error, it was instead a type error about nearbyTransit not being a StopsAssociated, so I added this to get more useful compilation errors in the future.
| @@ -121,7 +121,7 @@ struct AnnotatedMap: View { | |||
| .selected(isSelected) | |||
| .allowOverlap(true) | |||
| .allowOverlapWithPuck(true) | |||
| .visible(zoomLevel >= StopLayerGenerator.shared.stopZoomThreshold) | |||
| .visible(zoomLevel >= StopLayerGenerator.shared.stopZoomThreshold || isSelected) | |||
There was a problem hiding this comment.
This has been bothering me for a while lol
| import SwiftPhoenixClient | ||
| import SwiftUI | ||
|
|
||
| extension StopDetailsPage { |
There was a problem hiding this comment.
suggestion (non-blocking): I like this split out! Since this is all logic-focused, I think it could be worth moving it into a ViewModel. I think that would help with testability - you could test the ViewModel directly, and mock ViewModel function calls where helpful to unit test the view.
There was a problem hiding this comment.
Yeah that's a good idea! I can add that as an AC on the next ticket. edit: done
| // Adding a second bool argument here is a hack until we can remove the feature flag and set the new stop details | ||
| // entry directly, until then, we need a way to distinguish between entries coming from the map or not. |
Summary
Ticket: Combined stop + trip details: barebones UX
Stacked onto #554
This takes the existing trip details page and inserts it into the existing stop details page. Also updates the map to display the stop and vehicle whenever the vehicle filter changes.
- [ ] If you added any user facing strings on iOS, are they included in Localizable.xcstrings?Testing
WIP - I'm really deeply struggling with the tests, so I figured I should probably put this up for a first round of feedback while I slog through them.