refactor(iOS): Move stop details fetching into VM #599
Conversation
| screen: stopFilter != nil ? .stopDetailsFiltered : .stopDetailsUnfiltered | ||
| ) | ||
| } | ||
| .onDisappear { stopDetailsVM.leaveStopPredictions() } |
There was a problem hiding this comment.
suggestion(non-blocking): I think the calls to stopDetailsVM.handleStopAppear and leaveStopPredictions might feel more natural within the StopDetailsPage. Is there a benefit to having them a level higher?
There was a problem hiding this comment.
See my notes in the PR description, StopDetailsPage gets torn down and recreated every time the stop ID or stop filter changes, which was resulting in some issues. The onChange handles disconnecting and reconnecting from the channel, so we only actually want onAppear or onDisappear to be called when the page is changed entirely, not just changed to a different stop/filter.
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 StopDetailsPageContainer (stable) -> StopDetailsPage -> StopDetailsView.
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!
| } | ||
| // 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) } |
There was a problem hiding this comment.
question(non-blocking): can the stopId field be removed as a prop of StopDetailsPage entirely? That way it can't get out of sync with what is in the VM.
There was a problem hiding this comment.
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 StopData is only populated once the schedules have loaded, so StopDetailsPage couldn'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 in StopData, which I think was the fix for the issue I was seeing with double prediction loads.
iosApp/iosApp/ContentView.swift
Outdated
| .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 onAppear here.
* refactor(iOS): Move stop details fetching into VM * fix: Restore existing appear tracking
Summary
Ticket: Pre-work for Combined Stop + Trip Details: Show trip details when there's no predicted vehicle
This moves most of the logic remaining in the
StopDetailsPageinto theStopDetailsViewModel.I was running into some state issues when trying to implement the trip schedule display changes, and it got to the point where I thought these changes might resolve them. I'd been wanting to make these changes since doing something similar around moving all the trip details stuff into a separate struct, so they would have needed to get done eventually anyway.
While working through this, I also realized that the
StopDetailsPageonChange(of: stopId)was never being called, because the param values changing means the entire view is destroyed and recreated, resulting in onlyonDisappearthenonAppearbeing called. To resolve this, I moved theonChange(of: stopId),onAppear, andonDisappearall out ofStopDetailsPageand intoContentView, so that they can take advantage of the.id()on there and manage state outside of the destroyed view.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 resourceTesting
Manual testing, updated relevant existing tests to handle new structure