Updated STAR demo to support navigating to standard Android #275

kierse · 2022-11-11T03:36:18Z

This PR introduces LegacyNavigator as a solution for navigating out of a circuit environment. It also updates PetDetails to use this new approach when the user clicks on the "View full bio" button.

Note: instrumentation tests are not currently being run by CI. This will be addressed in a follow-up PR

Added legacy navigator to support navigating outside of Circuit. Reworked PetDetails "View full bio" external link to use new legacy navigator pattern.

kierse · 2022-11-15T18:47:59Z

samples/star/build.gradle.kts

@@ -32,7 +32,7 @@ android {
  defaultConfig {
    minSdk = 28
    testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner"
-    testApplicationId = "com.slack.circuit.star.androidTest"
+    testApplicationId = "com.slack.circuit.star.apk.androidTest"


Without this, the test suite can't find the test application at runtime

kierse · 2022-11-15T18:50:36Z

samples/star/build.gradle.kts

@@ -115,7 +115,7 @@ dependencies {
  testImplementation(libs.androidx.compose.ui.testing.manifest)
  testImplementation(libs.leakcanary.android.instrumentation)

-  debugImplementation(libs.androidx.compose.ui.testing.manifest)
+  androidTestImplementation(libs.androidx.compose.ui.testing.manifest)


Without this, ComposeActivity isn't merged with the manifest meaning it can't be found at runtime. Before introduction of the apk module, we used to support debug builds.

I think it needs to be present in both?

By both do you mean unit tests? We were already merging correctly for unit tests

kierse · 2022-11-15T18:52:20Z

samples/star/src/main/kotlin/com/slack/circuit/star/petdetail/PetDetail.kt

+@Composable
+private fun TopBar(state: PetDetailScreen.State) {
+  if (state !is PetDetailScreen.State.Success) return
+
+  CenterAlignedTopAppBar(title = { Text(state.name) }, navigationIcon = { BackPressNavIcon() })
+}
+
+@Composable
+private fun Loading(paddingValues: PaddingValues) {
+  Box(
+    modifier = Modifier.padding(paddingValues).fillMaxSize(),
+    contentAlignment = Alignment.Center
+  ) {
+    CircularProgressIndicator(modifier = Modifier.testTag(PROGRESS_TAG))
+  }
+}
+
+@Composable
+private fun UnknownAnimal(paddingValues: PaddingValues) {
+  Box(
+    modifier = Modifier.padding(paddingValues).fillMaxSize(),
+    contentAlignment = Alignment.Center
+  ) {
+    Text(
+      modifier = Modifier.testTag(UNKNOWN_ANIMAL_TAG),
+      text = stringResource(id = R.string.unknown_animals)
+    )
+  }
+}
+
+@Composable
+private fun ShowAnimal(
+  state: PetDetailScreen.State.Success,
+  padding: PaddingValues,
+) =
+  when (LocalConfiguration.current.orientation == Configuration.ORIENTATION_LANDSCAPE) {
+    true -> ShowAnimalLandscape(state, padding)
+    false -> ShowAnimalPortrait(state, padding)
+  }
+
+@Composable
+private fun ShowAnimalLandscape(state: PetDetailScreen.State.Success, padding: PaddingValues) {
+  Row(
+    modifier = Modifier.padding(padding),
+    horizontalArrangement = Arrangement.SpaceEvenly,
+  ) {
+    CircuitContent(
+      PetPhotoCarouselScreen(
+        name = state.name,
+        photoUrls = state.photoUrls,
+        photoUrlMemoryCacheKey = state.photoUrlMemoryCacheKey,
+      )
+    )
+    LazyColumn { petDetailDescriptions(state) }
+  }
+}
+
+@Composable
+private fun ShowAnimalPortrait(state: PetDetailScreen.State.Success, padding: PaddingValues) {
+  LazyColumn(
+    modifier = Modifier.padding(padding).testTag(ANIMAL_CONTAINER_TAG),
+    contentPadding = PaddingValues(16.dp),
+    verticalArrangement = Arrangement.spacedBy(16.dp),
+  ) {
+    item {
+      CircuitContent(
+        PetPhotoCarouselScreen(
+          name = state.name,
+          photoUrls = state.photoUrls,
+          photoUrlMemoryCacheKey = state.photoUrlMemoryCacheKey,
+        )
+      )
+    }
+    petDetailDescriptions(state)
+  }
+}
+


No change here. I just extracted it from PetDetail

iamritav · 2022-11-15T19:27:49Z

samples/star/src/androidTest/kotlin/com/slack/circuit/star/petdetail/PetDetailTest.kt

+        name = "Baxter",
+        description = "Grumpy looking Australian Terrier",
+        tags = listOf("dog", "terrier", "male"),
+        eventSink = channel::trySend


nit: why the name trySend for this callback? Should we just do send?

I can't use send here because it's suspend-able, trySend isn't

ZacSweers · 2022-11-15T20:37:11Z

samples/star/build.gradle.kts

@@ -115,7 +115,7 @@ dependencies {
  testImplementation(libs.androidx.compose.ui.testing.manifest)
  testImplementation(libs.leakcanary.android.instrumentation)

-  debugImplementation(libs.androidx.compose.ui.testing.manifest)
+  androidTestImplementation(libs.androidx.compose.ui.testing.manifest)


I think it needs to be present in both?

ZacSweers · 2022-11-15T20:38:01Z

samples/star/src/androidTest/kotlin/com/slack/circuit/star/petdetail/PetDetailTest.kt

+    composeTestRule.run {
+      setContent { CircuitCompositionLocals(circuitConfig) { PetDetail(success) } }
+
+      onNodeWithTag(CAROUSEL_TAG).assertIsDisplayed().performTouchInput { swipeUp() }
+      onNodeWithTag(FULL_BIO_TAG, true).assertIsDisplayed().performClick()
+
+      val event = channel.receive()
+      assertThat(event).isEqualTo(PetDetailScreen.Event.ViewFullBio(success.url))
+    }


Out of curiosity why do we do these run blocks?

Suggested change

composeTestRule.run {

setContent { CircuitCompositionLocals(circuitConfig) { PetDetail(success) } }

onNodeWithTag(CAROUSEL_TAG).assertIsDisplayed().performTouchInput { swipeUp() }

onNodeWithTag(FULL_BIO_TAG, true).assertIsDisplayed().performClick()

val event = channel.receive()

assertThat(event).isEqualTo(PetDetailScreen.Event.ViewFullBio(success.url))

}

composeTestRule.setContent {

setContent { CircuitCompositionLocals(circuitConfig) { PetDetail(success) } }

}

onNodeWithTag(CAROUSEL_TAG).assertIsDisplayed().performTouchInput { swipeUp() }

onNodeWithTag(FULL_BIO_TAG, true).assertIsDisplayed().performClick()

val event = channel.receive()

assertThat(event).isEqualTo(PetDetailScreen.Event.ViewFullBio(success.url))

All the onNodeWithX methods are extensions on SemanticsNodeInteractionsProvider which is implemented by the AndroidComposeTestRule (ie. composeTestRule). Without the run (or something similar) we'd need to prefix ever onNodeWithX with composeTestRule

ZacSweers · 2022-11-15T20:38:35Z

samples/star/src/androidTest/kotlin/com/slack/circuit/star/petdetail/PetDetailTest.kt

+    composeTestRule.run {
+      setContent { CircuitCompositionLocals(circuitConfig) { PetDetail(success) } }
+
+      onNodeWithTag(CAROUSEL_TAG).assertIsDisplayed().performTouchInput { swipeUp() }
+      onNodeWithTag(FULL_BIO_TAG, true).assertIsDisplayed().performClick()
+
+      val event = channel.receive()
+      assertThat(event).isEqualTo(PetDetailScreen.Event.ViewFullBio(success.url))
+    }


Out of curiosity why do we do these run blocks?

Suggested change

composeTestRule.run {

setContent { CircuitCompositionLocals(circuitConfig) { PetDetail(success) } }

onNodeWithTag(CAROUSEL_TAG).assertIsDisplayed().performTouchInput { swipeUp() }

onNodeWithTag(FULL_BIO_TAG, true).assertIsDisplayed().performClick()

val event = channel.receive()

assertThat(event).isEqualTo(PetDetailScreen.Event.ViewFullBio(success.url))

}

composeTestRule.setContent {

setContent { CircuitCompositionLocals(circuitConfig) { PetDetail(success) } }

}

onNodeWithTag(CAROUSEL_TAG).assertIsDisplayed().performTouchInput { swipeUp() }

onNodeWithTag(FULL_BIO_TAG, true).assertIsDisplayed().performClick()

val event = channel.receive()

assertThat(event).isEqualTo(PetDetailScreen.Event.ViewFullBio(success.url))

Same comment here

ZacSweers · 2022-11-15T20:40:05Z

Let's update the name to mention this is just a demo for navigating to standard android. "Legacy views" is referring to slack internal stuff, same with the jira slug 😅

kierse · 2022-11-15T22:50:42Z

Let's update the name to mention this is just a demo for navigating to standard android. "Legacy views" is referring to slack internal stuff, same with the jira slug 😅

Yes, good call. Done!

kierse requested a review from ZacSweers November 11, 2022 03:45

kierse force-pushed the ke/support-navigation-to-legacy-views branch 3 times, most recently from 03e912b to 7bf5ca5 Compare November 14, 2022 21:03

kierse changed the title ~~Ke/support navigation to legacy views~~ ANDINFRA-5316: support navigation to legacy views Nov 14, 2022

kierse force-pushed the ke/support-navigation-to-legacy-views branch from 7bf5ca5 to 1e1cd50 Compare November 15, 2022 16:35

Added legacy navigator

9978fe2

Added legacy navigator to support navigating outside of Circuit. Reworked PetDetails "View full bio" external link to use new legacy navigator pattern.

kierse force-pushed the ke/support-navigation-to-legacy-views branch from 3061d48 to 9978fe2 Compare November 15, 2022 18:39

kierse commented Nov 15, 2022

View reviewed changes

kierse marked this pull request as ready for review November 15, 2022 18:52

kierse requested review from jpetote and iamritav November 15, 2022 18:53

iamritav reviewed Nov 15, 2022

View reviewed changes

kierse requested a review from iamritav November 15, 2022 19:50

ZacSweers reviewed Nov 15, 2022

View reviewed changes

kierse changed the title ~~ANDINFRA-5316: support navigation to legacy views~~ Updated STAR demo to support navigating to standard Android Nov 15, 2022

Renamed custom navigator to better describe its purpose

a84ebed

kierse requested a review from ZacSweers November 15, 2022 22:53

Spotless cleanup

7485580

ZacSweers approved these changes Nov 15, 2022

View reviewed changes

kierse merged commit 047d73d into main Nov 15, 2022

kierse deleted the ke/support-navigation-to-legacy-views branch November 15, 2022 23:51

kierse mentioned this pull request Nov 21, 2022

Update backstack record screen when returning value #295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated STAR demo to support navigating to standard Android #275

Updated STAR demo to support navigating to standard Android #275

kierse commented Nov 11, 2022 •

edited

Loading

kierse Nov 15, 2022

kierse Nov 15, 2022

ZacSweers Nov 15, 2022

kierse Nov 15, 2022

kierse Nov 15, 2022 •

edited

Loading

iamritav Nov 15, 2022

kierse Nov 15, 2022

ZacSweers Nov 15, 2022

ZacSweers Nov 15, 2022

kierse Nov 15, 2022

ZacSweers Nov 15, 2022

kierse Nov 15, 2022

ZacSweers commented Nov 15, 2022

kierse commented Nov 15, 2022

Updated STAR demo to support navigating to standard Android #275

Updated STAR demo to support navigating to standard Android #275

Conversation

kierse commented Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kierse Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Nov 15, 2022

kierse commented Nov 15, 2022

kierse commented Nov 11, 2022 •

edited

Loading

kierse Nov 15, 2022 •

edited

Loading