-
Notifications
You must be signed in to change notification settings - Fork 3
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.TripDetails): Trip stop list UI #703
Conversation
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.
One minor comment, otherwise lgtm!
Modifier.alpha(0.6f), | ||
routeType = routeAccents.type, | ||
hideRealtimeIndicators = true | ||
Text( |
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.
The font looks a bit large here, I think we need to specify the fontSize as 17.sp
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.
I made some changes to the defined font styles in my vehicle card UI changes, I can also include this as part of that.
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.
This made me realize that the UpcomingTripView
font size is also too large, which will take a little more to resolve but I can include in this. bodyLarge
is defined in the Material defaults as 16.sp, but I overlooked that we're overriding those sizes in MyApplicationTheme
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.
I added the remaining text roles to our overrides, swapped titleLarge
(previously 20.sp) to titleSmall
, and added comments correlating them to the roles we use in Figma/iOS:
Typography(
bodyLarge = textStyle(fontSize = 24.sp),
bodyMedium = textStyle(fontSize = 17.sp), // Body
bodySmall = textStyle(fontSize = 16.sp),
headlineLarge = textStyle(fontSize = 17.sp, fontWeight = FontWeight.Bold), // Headline
headlineMedium = textStyle(fontSize = 16.sp), // Callout
headlineSmall = textStyle(fontSize = 15.sp), // Subheadline
titleLarge = textStyle(fontSize = 32.sp, fontWeight = FontWeight.Bold), // Title 1
titleMedium = textStyle(fontSize = 24.sp, fontWeight = FontWeight.Bold), // Title 2
titleSmall = textStyle(fontSize = 20.sp, fontWeight = FontWeight.SemiBold), // Title 3
labelLarge = textStyle(fontSize = 13.sp, fontWeight = FontWeight.Normal), // Footnote
labelMedium = textStyle(fontSize = 12.sp, fontWeight = FontWeight.Normal), // Caption 1
labelSmall = textStyle(fontSize = 11.sp, fontWeight = FontWeight.Normal), // Caption 2
)
Even though we use bodySmall
quite a bit, it doesn't really have a corresponding role in the designs, unless we want to double up on callout.
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.
Amazing, thanks @EmmaSimon!
@@ -107,14 +121,59 @@ fun TripStops( | |||
.defaultMinSize(minHeight = 48.dp), | |||
verticalAlignment = Alignment.CenterVertically | |||
) { | |||
AnimatedContent( |
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.
nice!
Summary
Ticket: 🤖 | stop + trip details | Trip details UI
What is this PR for?
This PR finishes off the UI for the trip stop list, including
Not included:
iOS
android
withContext(Dispatchers.Default)
where possibleTesting
What testing have you done?