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): show live data indicator on predictions #612

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

boringcactus
Copy link
Member

Summary

Ticket: 🤖 | Nearby | Realtime indicator

I tried making this a custom modifier like it is on iOS, but Android modifiers can't (as far as I was able to tell) just wrap the entire component with another Composable.

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

Testing

Manually verified (since testing the presence of semantically-invisible UI elements in Android is a bit difficult) that the icons show correctly and look the same way they do on iOS.

@boringcactus boringcactus requested a review from a team as a code owner December 24, 2024 21:24
Copy link
Contributor

@JackVCurtis JackVCurtis left a comment

Choose a reason for hiding this comment

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

👍

@@ -60,57 +60,65 @@ fun formatTime(time: Instant): String =
format.format(time.toLocalDateTime(TimeZone.currentSystemDefault()).toJavaLocalDateTime())

@Composable
fun UpcomingTripView(state: UpcomingTripViewState) {
fun UpcomingTripView(state: UpcomingTripViewState, hideRealtimeIndicators: Boolean = false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: maybe worth a test that the indicator image is included when expected

if (!hideIndicator) {
Image(
painterResource(R.drawable.live_data),
contentDescription = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

question(non-blocking): should there be a content description here, like "live"? This seems like an important symbol to convey to Talk Back users, though acknowledge that this is matching the existing iOS behavior

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.

3 participants