Skip to content
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

refactor(StopDetailsPage): Pull data into VM #661

Merged
merged 9 commits into from
Jan 21, 2025

Conversation

KaylaBrady
Copy link
Collaborator

@KaylaBrady KaylaBrady commented Jan 17, 2025

Summary

Ticket: 🤖 | stop + trip details | barebones UX

What is this PR for?

This PR moves the data management of the StopDetailsPage out of the component and into a new StopDetailsViewModel. This will be helpful for organization going forward and is key to resolving an issue with #653, where changing a filter within the stop details page (like tapping a route pill) caused the whole page to reload.

Heavily referenced StopDetailsViewModel.swift, pulling in the minimum logic necessary to resolve this one particular issue and unblock other work. We can pull more into the view model as we go.

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 resource
  • Expensive calculations are run in withContext(Dispatchers.Default) where possible

Testing

What testing have you done?

  • Ran locally and confirmed predictions, schedules all loaded on nearby transit & stop details when navigating back and forth.
  • Added unit tests

Comment on lines 43 to 41
val _stopDepartures = MutableStateFlow<StopDetailsDepartures?>(null)
val stopDepartures: StateFlow<StopDetailsDepartures?> = _stopDepartures
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Departures aren't stored in the iOS view model, but I think it makes things a bit easier to work with on the android side. The departures need to get passed from NearbyTransit => StopDetails so that they aren't recalculated & temporarily null when a stop filter changes, so may as well pass it on the VM.

*/

// https://stackoverflow.com/a/78911122
fun recordCurrentNavEntry(sheetRoute: SheetRoutes?) {
_currentNavEntry.value = sheetRoute

_currentNavEntry.update {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about update for atomically changing the value of state. This was more relevant for updating stopData, but also was convenient for updating the current nav entry & previous nav entry together.

import kotlinx.serialization.Serializable

@Serializable
data class StopData(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in shared code in case we want to unite it with the swift representation. Schedules are a required field in the swift side, but it was easier to make it optional on the android side & let the predictions & schedules load independently with the way we are using coroutines.

@KaylaBrady KaylaBrady marked this pull request as ready for review January 17, 2025 21:04
@KaylaBrady KaylaBrady requested a review from a team as a code owner January 17, 2025 21:04
@KaylaBrady KaylaBrady requested review from EmmaSimon and removed request for a team January 17, 2025 21:04
@KaylaBrady KaylaBrady force-pushed the kb-android-trip-details-barebones branch from 08171ca to 4436968 Compare January 21, 2025 15:57
@KaylaBrady KaylaBrady force-pushed the kb-stop-details-vm branch 2 times, most recently from fd7b230 to c333674 Compare January 21, 2025 16:21
Copy link
Contributor

@EmmaSimon EmmaSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a really good foundation!


class StopDetailsViewModel() : ViewModel() {}
class StopDetailsViewModel(
schedulesRepository: ISchedulesRepository,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this isn't also a private val?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't used as a property anywhere, so I got a suggestion not to mark it as a property.

@KaylaBrady KaylaBrady merged commit a96d9d8 into kb-android-trip-details-barebones Jan 21, 2025
7 checks passed
@KaylaBrady KaylaBrady deleted the kb-stop-details-vm branch January 21, 2025 18:04
@KaylaBrady KaylaBrady restored the kb-stop-details-vm branch January 21, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants