-
Notifications
You must be signed in to change notification settings - Fork 581
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
Update tiles sample for 1.1.0 #497
Conversation
@@ -40,15 +40,15 @@ class MessagingRepo(private val context: Context) { | |||
suspend fun updateContacts(contacts: List<Contact>) { | |||
context.dataStore.edit { | |||
it.clear() | |||
knownContacts.forEachIndexed { index, contact -> | |||
contacts.forEachIndexed { index, contact -> |
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 think a bug. No change in behavior since knownContacts
is what gets passed to this function from the outside, but for correctness, this function doesn't know about the knownContacts
property.
WearTilesKotlin/app/src/main/java/com/example/wear/tiles/messaging/MessagingRepo.kt
Show resolved
Hide resolved
WearTilesKotlin/app/src/main/java/com/example/wear/tiles/messaging/MessagingTileTheme.kt
Show resolved
Hide resolved
emptyClickable, | ||
deviceParameters | ||
) | ||
.setChipColors(ChipColors.primaryChipColors(MessagingTileTheme.colors)) |
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 ok?
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.
Yeah, I think it's the right approach for samples, unless we want to advocate for dynamically changing themes. Maybe for media, we should show that when artwork changes?
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.
Tangential: related to the repetition of context, context.getString(), deviceParameters, MessagingTileTheme.colors. This was something that I wanted to improve by having a Renderer type that could simplify these repetitions. https://github.com/google/horologist/blob/ab8ae486d43b55e46423979d9ad2aacf382d323c/tiles/src/main/java/com/google/android/horologist/tiles/render/SingleTileLayoutRenderer.kt
But not for here, maybe something we can review these and get a nice streamlined API.
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.
unless we want to advocate for dynamically changing themes. Maybe for media, we should show that when artwork changes?
yeah let's see how it looks/feels in practice when we do those samples
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.
Looks great
emptyClickable, | ||
deviceParameters | ||
) | ||
.setChipColors(ChipColors.primaryChipColors(MessagingTileTheme.colors)) |
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.
Yeah, I think it's the right approach for samples, unless we want to advocate for dynamically changing themes. Maybe for media, we should show that when artwork changes?
emptyClickable, | ||
deviceParameters | ||
) | ||
.setChipColors(ChipColors.primaryChipColors(MessagingTileTheme.colors)) |
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.
Tangential: related to the repetition of context, context.getString(), deviceParameters, MessagingTileTheme.colors. This was something that I wanted to improve by having a Renderer type that could simplify these repetitions. https://github.com/google/horologist/blob/ab8ae486d43b55e46423979d9ad2aacf382d323c/tiles/src/main/java/com/google/android/horologist/tiles/render/SingleTileLayoutRenderer.kt
But not for here, maybe something we can review these and get a nice streamlined API.
messagingTileLayout
fromMessagingTileRenderer