-
Notifications
You must be signed in to change notification settings - Fork 6
Introduce WorkflowContainer for running a workflow inside a Compose app. #16
Conversation
core-compose/src/main/java/com/squareup/workflow/ui/compose/WorkflowContainer.kt
Outdated
Show resolved
Hide resolved
99bf4fa to
e58d34d
Compare
core-compose/src/main/java/com/squareup/workflow/ui/compose/WorkflowContainer.kt
Show resolved
Hide resolved
rjrjr
left a comment
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.
Did you really expect me to say "hold up there pal, too many overloads"? I really like it.
!
|
What about the name, Maybe this should even be an extension on Workflow, like: @Composable fun App() {
MyWorkflow.render(props) { rendering ->Or even (and this is more consistent with @Composable fun App() {
val rendering: State<Foo> = MyWorkflow.renderAsState(props)
// and we get this sugar for free
val rendering: Foo by MyWorkflow.renderAsState(props)The return-value and lambda versions could also both live together. I will rename Asking for thoughts on the Kotlin slack too. |
core-compose/src/main/java/com/squareup/workflow/ui/compose/WorkflowContainer.kt
Outdated
Show resolved
Hide resolved
zach-klippenstein
left a comment
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.
Needs tests in core-compose.
| diagnosticListener: WorkflowDiagnosticListener? = null | ||
| ) { | ||
| WorkflowContainer(workflow, props, onOutput, modifier, diagnosticListener) { rendering -> | ||
| rendering.render(viewEnvironment) |
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.
This isn't applying the ComposeViewFactoryRoot. Since we're already in Compose land, it's not necessary to use that tool, but we should still handle that case correctly. Needs a test as well.
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.
Also, this should be applied in the non-ComposeWorkflow case as well. That's going to require changing that function to accept a ViewEnvironment, which is too bad. We could make ComposeViewFactory responsible about automatically applying the root wrapper. Going to do this in a separate PR first as a follow-up.
core-compose/src/main/java/com/squareup/workflow/ui/compose/WorkflowContainer.kt
Outdated
Show resolved
Hide resolved
core-compose/src/main/java/com/squareup/workflow/ui/compose/WorkflowContainer.kt
Outdated
Show resolved
Hide resolved
core-compose/src/main/java/com/squareup/workflow/ui/compose/WorkflowContainer.kt
Show resolved
Hide resolved
|
I'm happy to leave bikeshedding the name for later. (#22) |
e58d34d to
1367435
Compare
|
Added a bunch of tests, addressed the merge blockers. |
|
Github Actions run failed: Pushing a new commit to try again… |
1367435 to
14a6908
Compare
|
Actions failed again (second ID is |
|
https://www.githubstatus.com shows issues. |
14a6908 to
0707cd0
Compare
This is the third flavor of integration, it replaces `setContentWorkflow`, `WorkflowLayout`, `WorkflowRunnerViewModel`, etc., and makes it really easy to run a `Workflow` inside a pure Compose app. It's compatible with `ViewEnvironment`/`ViewRegistry`, but doesn't require it – it gives you the root rendering, and you can do whatever you want with it. It also supports running root `ComposeWorkflow`s directly, since they are self-rendering.
0707cd0 to
50bfb92
Compare
This is the third flavor of integration, it replaces
setContentWorkflow,WorkflowLayout,WorkflowRunnerViewModel, etc., and makes it really easy to run aWorkflowinside a pure Compose app. It's compatible withViewEnvironment/ViewRegistry, but doesn't require it – it gives you the root rendering, and you can do whatever you want with it.It also supports running root
ComposeWorkflows directly, since they are self-rendering.You can also use a
ViewRegistry, but it's entirely optional and there's no special API for it: