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.

Sign up for GitHub

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

Jump to bottom

[Jetcaster] Adding SupportingPaneScaffold to Home #1303

Merged
merged 10 commits into from
Apr 1, 2024
Merged

[Jetcaster] Adding SupportingPaneScaffold to Home #1303

merged 10 commits into from
Apr 1, 2024

Conversation

arriolac
Copy link
Contributor

@arriolac arriolac commented Mar 29, 2024 •
edited
Loading

Adding SupportingPaneScaffold to the home screen of Jetcaster.

Remaining TODOs:

  • Make expanded-width default to having the supporting pane be hidden
  • Hide top app bar in supporting pane when 2 panes are shown
Screen_recording_20240329_162553.webm
@arriolac arriolac changed the base branch from jetcaster/all_form_factors to chris/jetcaster/podcast_screen March 29, 2024 20:24
@florina-muntenescu
Copy link
Contributor

it looks like there's a padding between the 2 panes. can we remove that?
@arriolac
Copy link
Contributor Author

arriolac commented Apr 1, 2024 •
edited
Loading

it looks like there's a padding between the 2 panes. can we remove that?

Yep, I noticed that as well. Will remove.

EDIT: I think we actually want this spacing as calculated by the default scaffoldDirective. The color should be updated though to match the bg colors in both panes. I've address this in 2a65a17
Base automatically changed from chris/jetcaster/podcast_screen to jetcaster/all_form_factors April 1, 2024 18:28
@arriolac arriolac marked this pull request as ready for review April 1, 2024 18:28
@arriolac arriolac requested a review from a team as a code owner April 1, 2024 18:28
@arriolac arriolac changed the title [Jetcaster] WIP Adding SupportingPaneScaffold to Home Apr 1, 2024
jdkoren
jdkoren approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@jdkoren jdkoren left a comment

Choose a reason for hiding this comment

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

On tablet in landscape, I see a back arrow icon in the supporting pane after tapping on a podcast. Tapping this icon does nothing while in landscape, so it would be good to remove it -- happy to do that in a follow up PR if you prefer
Jetcaster/app/src/main/java/com/example/jetcaster/ui/home/Home.kt Outdated
@@ -103,30 +111,66 @@ import kotlinx.collections.immutable.PersistentList
import kotlinx.collections.immutable.toPersistentList
import kotlinx.coroutines.launch

@OptIn(ExperimentalMaterial3AdaptiveApi::class)
@Composable
fun Home(
navigateToPodcastDetails: (PodcastInfo) -> Unit,
Copy link
Contributor

Choose a reason for hiding this comment

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

Studio says this parameter is no longer used
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in afe8af7
@arriolac
Copy link
Contributor Author

arriolac commented Apr 1, 2024

On tablet in landscape, I see a back arrow icon in the supporting pane after tapping on a podcast. Tapping this icon does nothing while in landscape, so it would be good to remove it -- happy to do that in a follow up PR if you prefer

Yeah another PR would be great. I think the top bar should be hidden altogether when showing two panes
@arriolac arriolac merged commit 8223314 into jetcaster/all_form_factors Apr 1, 2024
1 check passed
@arriolac arriolac deleted the chris/jetcaster/camal branch April 1, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@arriolac @florina-muntenescu @jdkoren