-
Notifications
You must be signed in to change notification settings - Fork 226
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
Improving the Wear OS podcasts page #992
Conversation
Love this any chance it can be added to apple watch? |
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 good! Thanks so much for improving the theming! 🙇
private val viewModel: WearMainActivityViewModel by viewModels() | ||
|
||
override fun onCreate(savedInstanceState: Bundle?) { | ||
super.onCreate(savedInstanceState) | ||
setContent { | ||
val state by viewModel.state.collectAsState() | ||
WearAppTheme(theme.activeTheme) { | ||
WearAppTheme(Theme.ThemeType.EXTRA_DARK) { |
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 switch seems to have changed a bunch of the icons and text on multiple screens from white to a light gray, which doesn't look as good to me. For example, look at the Podcasts chip in the screenshot below.
Dark | Extra Dark |
---|---|
The figma designs didn't really follow our phone app colors, so there are a decent number of more hardcoded colors right now. That's why not all of the text changed to gray (for example).
Long story short, I started off trying to make the watch app use colors from our phone theme, but after a bit I realized that (1) they just weren't matching up well, (2) we very possibly will never allow theming the watch app, so I switching to more of just specifying the colors defined in Figma. Our handling of theming in the watch app definitely needs to be improved though. I'm appreciate you improving it.
I do think we should get the color of the text back to white, but it's not a blocker to merging this PR.
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 have tried to add a custom Wear OS theme instead to simplify this. The icons will now match the design and be white.
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.
There are definitely some improvements to be made but here is where it's at from the color and typography changes.
theme.mp4
state = listState, | ||
state = listState |
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.
Curious how you feel about trailing commas. I like them because they make the diffs smaller when adding/removing items, but it looks like you may not be a fan? Obviously this is a SUPER CRITICAL ISSUE!
I'll actually look into seeing if we can get spotless to enforce this one way or the other (although I like trailing commas, I'm fine having spotless enforce either approach).
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.
It would be good to enforce this. I don't mind either way which might show by the way I do both. 😁
Glad you're liking this @CookieyedCodes ! We'll do some UI updates to the apple watch app at some point, but we don't have any immediate plans. |
Ya fair enough, might as well wait for wwdc and see what they bring too watch for you guys :) |
val primaryText = Color.White | ||
val secondaryText = Color(0xFFBDC1C6) | ||
val upNextIcon = Color(0xFF4BA1D6) | ||
val downloadedIcon = Color(0xFF54C483) |
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 thought it might be nicer if we named these 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 is looking great! Thanks for the improvements.
Description
This change improves the podcasts page in the following ways.
It also includes:
Testing Instructions
Screenshots