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

feat(android.StopDetails): Shimmer loading #634

Merged
merged 6 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.mbta.tid.mbta_app.android.stopDetails
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.compose.ui.test.onNodeWithContentDescription
import androidx.compose.ui.test.onNodeWithText
import com.mbta.tid.mbta_app.model.LocationType
import com.mbta.tid.mbta_app.model.ObjectCollectionBuilder
Expand Down Expand Up @@ -126,4 +127,44 @@ class StopDetailsRoutesViewTest {
composeTestRule.onNodeWithText("Sample Headsign").assertExists()
composeTestRule.onNodeWithText("1 min").assertExists()
}

@Test
fun testLoadingStateFiltered() {
composeTestRule.setContent {
val filterState = remember {
mutableStateOf<StopDetailsFilter?>(StopDetailsFilter("routeId", 1))
}

StopDetailsRoutesView(
departures = null,
global = globalResponse,
now = now,
pinRoute = {},
pinnedRoutes = emptySet(),
filter = filterState.value,
updateStopFilter = filterState::value::set
)
}

composeTestRule.onNodeWithContentDescription("Loading...").assertExists()
}

@Test
fun testLoadingStateUnfiltered() {
composeTestRule.setContent {
val filterState = remember { mutableStateOf<StopDetailsFilter?>(null) }

StopDetailsRoutesView(
departures = null,
global = globalResponse,
now = now,
pinRoute = {},
pinnedRoutes = emptySet(),
filter = filterState.value,
updateStopFilter = filterState::value::set
)
}

composeTestRule.onNodeWithContentDescription("Loading...").assertExists()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import androidx.compose.ui.res.stringResource
import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.unit.sp
import com.mbta.tid.mbta_app.android.R
import com.mbta.tid.mbta_app.android.util.modifiers.placeholderIfLoading
import com.mbta.tid.mbta_app.model.Direction

private val localizedDirectionNames: Map<String, Int> =
Expand All @@ -35,14 +36,21 @@ fun DirectionLabel(direction: Direction, modifier: Modifier = Modifier) {
R.string.directionTo,
stringResource(directionNameFormatted(direction))
),
fontSize = 13.sp
fontSize = 13.sp,
modifier = Modifier.placeholderIfLoading()
)
Text(
destination,
fontSize = 17.sp,
fontWeight = FontWeight.SemiBold,
modifier = Modifier.placeholderIfLoading()
)
Text(destination, fontSize = 17.sp, fontWeight = FontWeight.SemiBold)
} else {
Text(
stringResource(directionNameFormatted(direction)),
fontSize = 17.sp,
fontWeight = FontWeight.SemiBold
fontWeight = FontWeight.SemiBold,
modifier = Modifier.placeholderIfLoading()
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import androidx.compose.ui.Modifier
import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.tooling.preview.Preview
import com.mbta.tid.mbta_app.android.MyApplicationTheme
import com.mbta.tid.mbta_app.android.util.placeholderIfLoading
import com.mbta.tid.mbta_app.android.util.modifiers.placeholderIfLoading
import com.mbta.tid.mbta_app.model.Alert
import com.mbta.tid.mbta_app.model.ObjectCollectionBuilder.Single.alert
import com.mbta.tid.mbta_app.model.ObjectCollectionBuilder.Single.route
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,15 @@ package com.mbta.tid.mbta_app.android.component
import androidx.compose.foundation.layout.Column
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.clearAndSetSemantics
import androidx.compose.ui.semantics.contentDescription
import com.mbta.tid.mbta_app.android.R
import com.mbta.tid.mbta_app.android.nearbyTransit.NearbyRouteView
import com.mbta.tid.mbta_app.android.util.modifiers.loadingShimmer
import com.mbta.tid.mbta_app.model.LoadingPlaceholders
import com.valentinilk.shimmer.shimmer
import kotlinx.datetime.Clock

@Composable
fun LoadingRouteCard() {
val placeholderRouteData = LoadingPlaceholders.nearbyRoute()

val contentDesc = stringResource(R.string.loading)

Column(
modifier = Modifier.shimmer().clearAndSetSemantics { contentDescription = contentDesc }
) {
Column(modifier = Modifier.loadingShimmer()) {
NearbyRouteView(placeholderRouteData, false, {}, Clock.System.now(), { _, _ -> })
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.unit.dp
import com.mbta.tid.mbta_app.android.R
import com.mbta.tid.mbta_app.android.util.placeholderIfLoading
import com.mbta.tid.mbta_app.android.util.modifiers.placeholderIfLoading

@Composable
fun PinButton(pinned: Boolean, color: Color, action: () -> Unit) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import androidx.compose.ui.res.stringResource
import androidx.compose.ui.unit.dp
import com.mbta.tid.mbta_app.android.R
import com.mbta.tid.mbta_app.android.generated.drawableByName
import com.mbta.tid.mbta_app.android.util.placeholderIfLoading
import com.mbta.tid.mbta_app.android.util.modifiers.placeholderIfLoading
import com.mbta.tid.mbta_app.model.RealtimePatterns
import com.mbta.tid.mbta_app.model.Route

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import androidx.compose.ui.semantics.semantics
import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import com.mbta.tid.mbta_app.android.util.placeholderIfLoading
import com.mbta.tid.mbta_app.android.util.modifiers.placeholderIfLoading

@Composable
fun TransitHeader(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import com.mbta.tid.mbta_app.android.R
import com.mbta.tid.mbta_app.android.util.UpcomingTripAccessibilityFormatters
import com.mbta.tid.mbta_app.android.util.placeholderIfLoading
import com.mbta.tid.mbta_app.android.util.modifiers.placeholderIfLoading
import com.mbta.tid.mbta_app.android.util.typeText
import com.mbta.tid.mbta_app.model.Alert
import com.mbta.tid.mbta_app.model.RealtimePatterns
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import androidx.compose.ui.res.colorResource
import androidx.compose.ui.unit.dp
import com.mbta.tid.mbta_app.android.R
import com.mbta.tid.mbta_app.android.component.StopDeparturesSummaryList
import com.mbta.tid.mbta_app.android.util.placeholderIfLoading
import com.mbta.tid.mbta_app.android.util.modifiers.placeholderIfLoading
import com.mbta.tid.mbta_app.model.PatternsByStop
import com.mbta.tid.mbta_app.model.StopDetailsFilter
import com.mbta.tid.mbta_app.model.TripInstantDisplay
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.compositionLocalOf
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
Expand All @@ -26,6 +25,7 @@ import com.mbta.tid.mbta_app.android.component.ErrorBannerViewModel
import com.mbta.tid.mbta_app.android.component.LoadingRouteCard
import com.mbta.tid.mbta_app.android.state.getSchedule
import com.mbta.tid.mbta_app.android.state.subscribeToPredictions
import com.mbta.tid.mbta_app.android.util.IsLoadingSheetContents
import com.mbta.tid.mbta_app.android.util.managePinnedRoutes
import com.mbta.tid.mbta_app.android.util.rememberSuspend
import com.mbta.tid.mbta_app.android.util.timer
Expand All @@ -40,8 +40,6 @@ import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import org.koin.androidx.compose.koinViewModel

val IsLoadingSheetContents = compositionLocalOf { false }

@Composable
fun NearbyTransitView(
alertData: AlertsStreamDataResponse?,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
package com.mbta.tid.mbta_app.android.stopDetails

import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.colorResource
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import com.mbta.tid.mbta_app.android.MyApplicationTheme
import com.mbta.tid.mbta_app.android.R
import com.mbta.tid.mbta_app.android.util.IsLoadingSheetContents
import com.mbta.tid.mbta_app.android.util.modifiers.loadingShimmer
import com.mbta.tid.mbta_app.model.LoadingPlaceholders
import com.mbta.tid.mbta_app.model.ObjectCollectionBuilder
import com.mbta.tid.mbta_app.model.PatternsByStop
import com.mbta.tid.mbta_app.model.Prediction
Expand All @@ -26,15 +33,46 @@ import kotlinx.datetime.Instant

@Composable
fun StopDetailsRoutesView(
departures: StopDetailsDepartures,
departures: StopDetailsDepartures?,
global: GlobalResponse?,
now: Instant,
filter: StopDetailsFilter?,
pinRoute: (String) -> Unit,
pinnedRoutes: Set<String>,
updateStopFilter: (StopDetailsFilter?) -> Unit
) {
if (filter != null) {
if (departures == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could simplify this code by making the variables we need to change conditional on their own booleans, and having a single non-nested if statement. E.g.

val colModifier = if (departures == null) { Modifier.loadingShimmer() } else { Modifier }
val populatedPatternsByStop = if (departures == null) { LoadingPlaceholders.patternsByStop() } else { patternsByStop }

if (filter != null) {
 Column(colModifier) {
   StopDetailsRouteView(populatedPatternsByStop)
  }
} else {
 StopDetailsFilteredRouteView(populatedPatternsByStop)
}

I find nested if statements tough to parse, personally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Point taken on readability! of neseted ifs! I opted to break out a distinct LoadingStopDetailsView in dd7e915 for consistency with LoadingRouteCard which also removes the nesting.

Column {
CompositionLocalProvider(IsLoadingSheetContents provides true) {
Column(modifier = Modifier.loadingShimmer()) {
if (filter == null) {
Column(
modifier =
Modifier.verticalScroll(rememberScrollState())
.padding(8.dp)
.weight(1f)
) {
val placeholderPatterns = LoadingPlaceholders.patternsByStop()
StopDetailsRouteView(
LoadingPlaceholders.patternsByStop(),
now,
pinned = pinnedRoutes.contains(placeholderPatterns.routeIdentifier),
onPin = {},
{}
)
}
} else {
StopDetailsFilteredRouteView(
departures = LoadingPlaceholders.stopDetailsDepartures(filter),
global = global,
now = now,
filter = filter
) {}
}
}
}
}
} else if (filter != null) {
StopDetailsFilteredRouteView(
departures = departures,
global = global,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import androidx.compose.foundation.border
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.material3.CircularProgressIndicator
import androidx.compose.material3.HorizontalDivider
import androidx.compose.runtime.Composable
import androidx.compose.runtime.remember
Expand Down Expand Up @@ -92,18 +91,14 @@ fun StopDetailsView(

ErrorBanner(errorBannerViewModel)

if (departures != null) {
StopDetailsRoutesView(
departures,
globalResponse,
now,
filter ?: departures.autoStopFilter(),
togglePinnedRoute,
pinnedRoutes,
updateStopFilter
)
} else {
CircularProgressIndicator()
}
StopDetailsRoutesView(
departures,
globalResponse,
now,
filter ?: departures?.autoStopFilter(),
togglePinnedRoute,
pinnedRoutes,
updateStopFilter
)
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.mbta.tid.mbta_app.android.util

import android.app.Activity
import androidx.compose.runtime.compositionLocalOf
import androidx.compose.runtime.staticCompositionLocalOf
import com.google.android.gms.location.FusedLocationProviderClient

Expand All @@ -11,3 +12,9 @@ val LocalLocationClient =
staticCompositionLocalOf<FusedLocationProviderClient> {
throw IllegalStateException("no location client")
}

/**
* Whether the sheet contents are loading. Used by low-level child views to determine whether they
* should be styled as placeholders by `Modifier.placeholderIfLoading()`
*/
val IsLoadingSheetContents = compositionLocalOf { false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package com.mbta.tid.mbta_app.android.util
package com.mbta.tid.mbta_app.android.util.modifiers

import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import com.eygraber.compose.placeholder.material3.placeholder
import com.mbta.tid.mbta_app.android.nearbyTransit.IsLoadingSheetContents
import com.mbta.tid.mbta_app.android.util.IsLoadingSheetContents

@Composable
fun Modifier.placeholderIfLoading(): Modifier {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.mbta.tid.mbta_app.android.util.modifiers

import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.clearAndSetSemantics
import androidx.compose.ui.semantics.contentDescription
import com.mbta.tid.mbta_app.android.R
import com.valentinilk.shimmer.shimmer

@Composable
fun Modifier.loadingShimmer(): Modifier {
val contentDesc = stringResource(R.string.loading)
return this then Modifier.shimmer().clearAndSetSemantics { contentDescription = contentDesc }
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@
<string name="other_link_source_code">Ver código fuente en GitHub</string>
<string name="other_link_tos">Términos de uso</string>
<string name="outbound">Saliente</string>
<string name="recently_viewed">Visto recientemente</string>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alphabetized in conflict resolution

<string name="refresh">Refrescar</string>
<string name="refresh_predictions">Refrescar predicciones</string>
<string name="reload_data">Recargar datos</string>
<string name="recently_viewed">Visto recientemente</string>
<string name="resources_link_fare_info">Información de tarifas</string>
<string name="resources_link_mticket">Boletos de tren de cercanías y ferri</string>
<string name="resources_link_mticket_note">mTicket App</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@
<string name="other_link_source_code">Gade sous sou GitHub</string>
<string name="other_link_tos">Kondisyon itilizasyon</string>
<string name="outbound">Soti</string>
<string name="recently_viewed">Sa w fenk sot Wè</string>
<string name="refresh">Rafrechi</string>
<string name="refresh_predictions">Rafrechi prediksyon</string>
<string name="reload_data">Rechaje done</string>
<string name="recently_viewed">Sa w fenk sot Wè</string>
<string name="resources_link_fare_info">Enfòmasyon Tarif</string>
<string name="resources_link_mticket">Tikè Tren Vil la ak Bato</string>
<string name="resources_link_mticket_note">Aplikasyon mTicket</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@
<string name="other_link_source_code">Exibir origem no GitHub</string>
<string name="other_link_tos">Termos de Uso</string>
<string name="outbound">Saída</string>
<string name="recently_viewed">Visto(s) recentemente</string>
<string name="refresh">Atualizar</string>
<string name="refresh_predictions">Atualizar previsões</string>
<string name="reload_data">Recarregar dados</string>
<string name="recently_viewed">Visto(s) recentemente</string>
<string name="resources_link_fare_info">Informações sobre tarifas</string>
<string name="resources_link_mticket">Bilhetes de transporte diário por trem e balsa</string>
<string name="resources_link_mticket_note">Aplicativo mTicket</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@
<string name="other_link_source_code">Xem nguồn trên GitHub</string>
<string name="other_link_tos">Điều khoản sử dụng</string>
<string name="outbound">Đi</string>
<string name="recently_viewed">Đã xem gần đây</string>
<string name="refresh">Làm mới</string>
<string name="refresh_predictions">Làm mới dự đoán</string>
<string name="reload_data">Tải lại dữ liệu</string>
<string name="recently_viewed">Đã xem gần đây</string>
<string name="resources_link_fare_info">Thông tin giá vé</string>
<string name="resources_link_mticket">Vé phà vé đường sắt ngoại ô (Commuter Rail)</string>
<string name="resources_link_mticket_note">Ứng dụng mTicket</string>
Expand Down
Loading
Loading