refactor(iOS): Move stop details fetching into VM #599
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| import SwiftPhoenixClient | ||
| import SwiftUI | ||
|
|
||
| struct ContentView: View { | ||
|
Check notice on line 7 in iosApp/iosApp/ContentView.swift
|
||
| @Environment(\.scenePhase) private var scenePhase | ||
|
|
||
| let platform = Platform_iosKt.getPlatform().name | ||
|
|
@@ -286,16 +286,18 @@ | |
| viewportProvider: viewportProvider | ||
| ) | ||
| .toolbar(.hidden, for: .tabBar) | ||
| } | ||
| // Set id per stop so that transitioning from one stop to another is handled by removing | ||
| // the existing stop view & creating a new one | ||
| .id(stopId) | ||
| .onChange(of: stopId) { nextStopId in stopDetailsVM.handleStopChange(nextStopId) } | ||
| .onAppear { | ||
| stopDetailsVM.handleStopAppear(stopId) | ||
| screenTracker.track( | ||
| screen: stopFilter != nil ? .stopDetailsFiltered : .stopDetailsUnfiltered | ||
|
There was a problem hiding this comment. Sorry @EmmaSimon one last thought - does this affect how visits to stop details are counted in GA? Wondering if it would alter the stats about how users progress through the funnel There was a problem hiding this comment. I think it might actually, I should move it back to where it was and add a second |
||
| ) | ||
| } | ||
| .onDisappear { stopDetailsVM.leaveStopPredictions() } | ||
|
There was a problem hiding this comment. suggestion(non-blocking): I think the calls to There was a problem hiding this comment. See my notes in the PR description, There was a problem hiding this comment. Oh right, thank you, I wasn't connecting the dots on how this fit in with that. One other non-blocking thought that comes to mind based on that - it could help to clarify the behavior to add a new component to the stop details hierarchy to clarify w/ naming and/or comments that there is this stable stop details container that is unchanging and presented once, and that the inner piece is more volatile. Something like Page vs Container doesn't feel meaningfully distinctive as a name, but I think clarifying the difference between these layers could help with understandability down the line. There was a problem hiding this comment. Yeah I like that idea, I can try to fit that into one of the follow up PRs! |
||
| .transition(transition) | ||
|
|
||
| case let .legacyStopDetails(stop, filter): | ||
|
|
@@ -357,7 +359,7 @@ | |
| } | ||
| } | ||
|
|
||
| private func recenterOnVehicleButtonInfo() -> (RouteType, Vehicle, Stop)? { | ||
| guard case let .stopDetails(stopId: _, stopFilter: stopFilter, tripFilter: tripFilter) = nearbyVM | ||
| .navigationStack.lastSafe(), | ||
| let selectedVehicle = mapVM.selectedVehicle, tripFilter?.vehicleId == selectedVehicle.id, | ||
|
|
||
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): can the
stopIdfield be removed as a prop ofStopDetailsPageentirely? That way it can't get out of sync with what is in the VM.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.
Oh interesting. That does seem temping, but I don't think it's possible, it might cause issues with the loading state and page transitions. The ID in
StopDatais only populated once the schedules have loaded, soStopDetailsPagecouldn't start checking auto filters until after the network request, and it uses the prop stop ID to make sure that it doesn't change the departures if it doesn't match the ID inStopData, which I think was the fix for the issue I was seeing with double prediction loads.