-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Jetcaster] Improve scrolling in Home screen #1274
Conversation
239c2cb
to
18fb54e
Compare
Changes: * Support scrolling entire content in home screen * Fix `PreviewHomeContent` * Remove non-screen composable view model usages
18fb54e
to
836c888
Compare
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.
Just some small nits, feel free to fix up and then merge
if (featuredPodcasts.isNotEmpty()) { | ||
Spacer(Modifier.height(16.dp)) | ||
stickyHeader { | ||
if (homeCategories.isNotEmpty()) { |
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.
nit: It should be the other way around I think?
if (homeCategories.isNotEmpty()) {
stickyHeader {}
}
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.
Good catch. Fixed it
) | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
nit: This is super nested, consider breaking it apart into multiple composables where it makes sense
.windowInsetsTopHeight(WindowInsets.statusBars) | ||
) | ||
val scrimColor = MaterialTheme.colors.primary.copy(alpha = 0.38f) | ||
Scaffold( |
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.
Do we even need the Scaffold? Looks like it is sitting in a Column already that maybe could just be used seeing as its just a TopAppBar and then LazyColumn
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 was thinking a Scaffold would be good if we want the top app bar to dismiss while scrolling. Since I haven't implemented that yet (requires M3) I'll remove the Scaffold for now 👍
Changes:
PreviewHomeContent
It would be great to also support collapsing the toolbar while scrolling but that might be better accomplished when migrating Jetcaster to Material3.
jetcaster_scrolling.webm