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.

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

Spike compose support #177

Draft
wants to merge 6 commits into
base: displayable-generic
Choose a base branch
from
Draft

Conversation

ryanmoelter
Copy link
Contributor

@ryanmoelter ryanmoelter commented May 2, 2021

This is the gist of my idea for supporting compose. Still a ways to go. :)

Done:

  • Create ComposeStep
  • Create some compose <-> View interop support
  • Create compose-friendly Navigator and Journey

Not done:

  • See if we can keep the minSdkLevel low for non-compose packages
  • Add tests

@ryanmoelter ryanmoelter marked this pull request as draft May 4, 2021 02:48
@ryanmoelter ryanmoelter changed the base branch from displayable-generic to master June 24, 2021 18:23
@ryanmoelter ryanmoelter changed the base branch from master to displayable-generic July 21, 2021 16:33

internal class IntroStep(
private val goToLearnMore: () -> Unit
) : Step<IntroBinding>(IntroBinding::inflate) {
) : ComposeStep() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first ComposeStep!

Comment on lines +18 to 23
@Composable
internal fun IntroStepContent(goToLearnMore: () -> Unit) {
Button(onClick = goToLearnMore) {
Text("Go to learn more")
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, this probably belongs inside the Step. That way, it can access the data inside the Step, which we can fake for the previews.

class MyStep : ComposeStep() {
  val myDataFlow = StateFlow<MyData>()

  @Composable
  override fun Compose() {
    val myData = myDataFlow.collectAsState()
    // Use myData here
  }
}

Comment on lines 5 to 11
public class SimpleComposeStep(public val Content: @Composable SimpleComposeStep.() -> Unit) : ComposeStep() {

@Composable
override fun Compose() {
this.Content()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be useful for static content, but that doesn't happen often.

Comment on lines +13 to +14
// See https://issuetracker.google.com/issues/176079157#comment11
implementation "org.jetbrains.kotlin:kotlin-gradle-plugin:1.5.10"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figuring this out took literally TWO AND A HALF MONTHS. Everything else worked from the start.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's pretty annoying.

@@ -9,18 +9,22 @@ repositories {
}

dependencies {
implementation "com.android.tools.build:gradle:4.1.1"
implementation 'com.android.tools.build:gradle:7.0.0-beta05'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since AS arctic fox has been released, we can bump this to the stable version. Same with compose 🥳

}

compileOptions {
setSourceCompatibility(JavaVersion.VERSION_1_8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to bump this to Java 11 for compatibility with arctic fox.

}
}

@Preview(device = Devices.PIXEL_3, name = "Intro step")
@Composable
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably encourage the pattern on annotating the compose function with the preview annotation so that you can look at how the entire step view will look like and if we want to look at other smaller functions we can do that too. Creating a separate method just for the preview might create misdirection when reading the code.

})
}

public class ComposeStepWrapper(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the naming of the classes is a bit confusing. But I can't think of better names. Something we might want to revisit later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants