Merge pull request #1681 from android/dt/improve-navigation

Fix topic chip navigation from ForYou screen

Author: dturner

app-nia-catalog/dependencies/releaseRuntimeClasspath.txt

@@ -1,6 +1,6 @@
-androidx.activity:activity-compose:1.9.2
-androidx.activity:activity-ktx:1.9.2
-androidx.activity:activity:1.9.2
+androidx.activity:activity-compose:1.9.3
+androidx.activity:activity-ktx:1.9.3
+androidx.activity:activity:1.9.3
 androidx.annotation:annotation-experimental:1.4.0
 androidx.annotation:annotation-jvm:1.8.0
 androidx.annotation:annotation:1.8.0

app/dependencies/prodReleaseRuntimeClasspath.txt

@@ -1,6 +1,6 @@
-androidx.activity:activity-compose:1.9.2
-androidx.activity:activity-ktx:1.9.2
-androidx.activity:activity:1.9.2
+androidx.activity:activity-compose:1.9.3
+androidx.activity:activity-ktx:1.9.3
+androidx.activity:activity:1.9.3
 androidx.annotation:annotation-experimental:1.4.1
 androidx.annotation:annotation-jvm:1.8.1
 androidx.annotation:annotation:1.8.1
@@ -196,11 +196,11 @@ com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava
 com.google.j2objc:j2objc-annotations:1.3
 com.google.protobuf:protobuf-javalite:4.26.1
 com.google.protobuf:protobuf-kotlin-lite:4.26.1
-com.squareup.retrofit2:converter-kotlinx-serialization:2.11.0
 com.squareup.okhttp3:logging-interceptor:4.12.0
 com.squareup.okhttp3:okhttp:4.12.0
 com.squareup.okio:okio-jvm:3.9.0
 com.squareup.okio:okio:3.9.0
+com.squareup.retrofit2:converter-kotlinx-serialization:2.11.0
 com.squareup.retrofit2:retrofit:2.11.0
 io.coil-kt:coil-base:2.7.0
 io.coil-kt:coil-compose-base:2.7.0

app/src/androidTest/kotlin/com/google/samples/apps/nowinandroid/ui/NavigationTest.kt

@@ -16,13 +16,16 @@
 
 package com.google.samples.apps.nowinandroid.ui
 
+import androidx.compose.ui.semantics.SemanticsActions.ScrollBy
 import androidx.compose.ui.test.assertCountEquals
 import androidx.compose.ui.test.assertIsOn
 import androidx.compose.ui.test.assertIsSelected
 import androidx.compose.ui.test.hasTestTag
 import androidx.compose.ui.test.hasText
 import androidx.compose.ui.test.junit4.createAndroidComposeRule
+import androidx.compose.ui.test.onAllNodesWithTag
 import androidx.compose.ui.test.onAllNodesWithText
+import androidx.compose.ui.test.onFirst
 import androidx.compose.ui.test.onNodeWithContentDescription
 import androidx.compose.ui.test.onNodeWithTag
 import androidx.compose.ui.test.onNodeWithText
@@ -32,6 +35,7 @@ import androidx.test.espresso.Espresso
 import androidx.test.espresso.NoActivityResumedException
 import com.google.samples.apps.nowinandroid.MainActivity
 import com.google.samples.apps.nowinandroid.R
+import com.google.samples.apps.nowinandroid.core.data.repository.NewsRepository
 import com.google.samples.apps.nowinandroid.core.data.repository.TopicsRepository
 import com.google.samples.apps.nowinandroid.core.model.data.Topic
 import com.google.samples.apps.nowinandroid.core.rules.GrantPostNotificationsPermissionRule
@@ -75,6 +79,9 @@ class NavigationTest {
     @Inject
     lateinit var topicsRepository: TopicsRepository
 
+    @Inject
+    lateinit var newsRepository: NewsRepository
+
     // The strings used for matching in these tests
     private val navigateUp by composeTestRule.stringResource(FeatureForyouR.string.feature_foryou_navigate_up)
     private val forYou by composeTestRule.stringResource(FeatureForyouR.string.feature_foryou_title)
@@ -267,4 +274,44 @@ class NavigationTest {
             onNodeWithTag("topic:${topic.id}").assertExists()
         }
     }
+
+    @Test
+    fun navigatingToTopicFromForYou_showsTopicDetails() {
+        composeTestRule.apply {
+            // Get the first news resource
+            val newsResource = runBlocking {
+                newsRepository.getNewsResources().first().first()
+            }
+
+            // Get its first topic and follow it
+            val topic = newsResource.topics.first()
+            onNodeWithText(topic.name).performClick()
+
+            // Get the news feed and scroll to the news resource
+            // Note: Possible flakiness. If the content of the news resource is long then the topic
+            // tag might not be visible meaning it cannot be clicked
+            onNodeWithTag("forYou:feed")
+                .performScrollToNode(hasTestTag("newsResourceCard:${newsResource.id}"))
+                .fetchSemanticsNode()
+                .apply {
+                    val newsResourceCardNode = onNodeWithTag("newsResourceCard:${newsResource.id}")
+                        .fetchSemanticsNode()
+                    config[ScrollBy].action?.invoke(
+                        0f,
+                        // to ensure the bottom of the card is visible,
+                        // manually scroll the difference between the height of
+                        // the scrolling node and the height of the card
+                        (newsResourceCardNode.size.height - size.height).coerceAtLeast(0).toFloat(),
+                    )
+                }
+
+            // Click the first topic tag
+            onAllNodesWithTag("topicTag:${topic.id}", useUnmergedTree = true)
+                .onFirst()
+                .performClick()
+
+            // Verify that we're on the correct topic details screen
+            onNodeWithTag("topic:${topic.id}").assertExists()
+        }
+    }
 }

app/src/main/kotlin/com/google/samples/apps/nowinandroid/navigation/NiaNavHost.kt

@@ -20,10 +20,12 @@ import androidx.compose.runtime.Composable
 import androidx.compose.ui.Modifier
 import androidx.navigation.compose.NavHost
 import com.google.samples.apps.nowinandroid.feature.bookmarks.navigation.bookmarksScreen
-import com.google.samples.apps.nowinandroid.feature.foryou.navigation.ForYouRoute
-import com.google.samples.apps.nowinandroid.feature.foryou.navigation.forYouScreen
+import com.google.samples.apps.nowinandroid.feature.foryou.navigation.ForYouBaseRoute
+import com.google.samples.apps.nowinandroid.feature.foryou.navigation.forYouSection
 import com.google.samples.apps.nowinandroid.feature.interests.navigation.navigateToInterests
 import com.google.samples.apps.nowinandroid.feature.search.navigation.searchScreen
+import com.google.samples.apps.nowinandroid.feature.topic.navigation.navigateToTopic
+import com.google.samples.apps.nowinandroid.feature.topic.navigation.topicScreen
 import com.google.samples.apps.nowinandroid.navigation.TopLevelDestination.INTERESTS
 import com.google.samples.apps.nowinandroid.ui.NiaAppState
 import com.google.samples.apps.nowinandroid.ui.interests2pane.interestsListDetailScreen
@@ -44,10 +46,18 @@ fun NiaNavHost(
     val navController = appState.navController
     NavHost(
         navController = navController,
-        startDestination = ForYouRoute,
+        startDestination = ForYouBaseRoute,
         modifier = modifier,
     ) {
-        forYouScreen(onTopicClick = navController::navigateToInterests)
+        forYouSection(
+            onTopicClick = navController::navigateToTopic,
+        ) {
+            topicScreen(
+                showBackButton = true,
+                onBackClick = navController::popBackStack,
+                onTopicClick = navController::navigateToTopic,
+            )
+        }
         bookmarksScreen(
             onTopicClick = navController::navigateToInterests,
             onShowSnackbar = onShowSnackbar,

app/src/main/kotlin/com/google/samples/apps/nowinandroid/navigation/TopLevelDestination.kt

@@ -21,6 +21,7 @@ import androidx.compose.ui.graphics.vector.ImageVector
 import com.google.samples.apps.nowinandroid.R
 import com.google.samples.apps.nowinandroid.core.designsystem.icon.NiaIcons
 import com.google.samples.apps.nowinandroid.feature.bookmarks.navigation.BookmarksRoute
+import com.google.samples.apps.nowinandroid.feature.foryou.navigation.ForYouBaseRoute
 import com.google.samples.apps.nowinandroid.feature.foryou.navigation.ForYouRoute
 import com.google.samples.apps.nowinandroid.feature.interests.navigation.InterestsRoute
 import kotlin.reflect.KClass
@@ -29,23 +30,34 @@ import com.google.samples.apps.nowinandroid.feature.foryou.R as forYouR
 import com.google.samples.apps.nowinandroid.feature.search.R as searchR
 
 /**
- * Type for the top level destinations in the application. Each of these destinations
- * can contain one or more screens (based on the window size). Navigation from one screen to the
- * next within a single destination will be handled directly in composables.
+ * Type for the top level destinations in the application. Contains metadata about the destination
+ * that is used in the top app bar and common navigation UI.
+ *
+ * @param selectedIcon The icon to be displayed in the navigation UI when this destination is
+ * selected.
+ * @param unselectedIcon The icon to be displayed in the navigation UI when this destination is
+ * not selected.
+ * @param iconTextId Text that to be displayed in the navigation UI.
+ * @param titleTextId Text that is displayed on the top app bar.
+ * @param route The route to use when navigating to this destination.
+ * @param baseRoute The highest ancestor of this destination. Defaults to [route], meaning that
+ * there is a single destination in that section of the app (no nested destinations).
  */
 enum class TopLevelDestination(
     val selectedIcon: ImageVector,
     val unselectedIcon: ImageVector,
     @StringRes val iconTextId: Int,
     @StringRes val titleTextId: Int,
     val route: KClass<*>,
+    val baseRoute: KClass<*> = route,
 ) {
     FOR_YOU(
         selectedIcon = NiaIcons.Upcoming,
         unselectedIcon = NiaIcons.UpcomingBorder,
         iconTextId = forYouR.string.feature_foryou_title,
         titleTextId = R.string.app_name,
         route = ForYouRoute::class,
+        baseRoute = ForYouBaseRoute::class,
     ),
     BOOKMARKS(
         selectedIcon = NiaIcons.Bookmarks,

app/src/main/kotlin/com/google/samples/apps/nowinandroid/ui/NiaApp.kt

@@ -152,7 +152,7 @@ internal fun NiaApp(
             appState.topLevelDestinations.forEach { destination ->
                 val hasUnread = unreadDestinations.contains(destination)
                 val selected = currentDestination
-                    .isRouteInHierarchy(destination.route)
+                    .isRouteInHierarchy(destination.baseRoute)
                 item(
                     selected = selected,
                     onClick = { appState.navigateToTopLevelDestination(destination) },

app/src/main/kotlin/com/google/samples/apps/nowinandroid/ui/NiaAppState.kt

@@ -90,7 +90,7 @@ class NiaAppState(
     val currentTopLevelDestination: TopLevelDestination?
         @Composable get() {
             return TopLevelDestination.entries.firstOrNull { topLevelDestination ->
-                currentDestination?.hasRoute(route = topLevelDestination.route) ?: false
+                currentDestination?.hasRoute(route = topLevelDestination.route) == true
             }
         }