This repository was archived by the owner on Feb 5, 2021. It is now read-only.
Make renderAsState public, make WorkflowContainer take a ViewEnvironment. #32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjrjr
approved these changes
May 28, 2020
zach-klippenstein
force-pushed
the
zachklipp/renderasstate
branch
from
8e3b85c to
9978a18
Compare
zach-klippenstein
force-pushed
the
zachklipp/renderasstate
branch
from
9978a18 to
14148a8
Compare
…ent. This narrows the scope of `WorkflowContainer` to be only for showing a workflow's renderings using a `ViewEnvironment`. This elimnates the need to have separate overloads for workflows with rendering type `ComposeRendering`, and is more idiomatic – the old `WorkflowContainer` wasn't a "container" in any Compose-y sense of the word, and it was really weird that it took a content function. Now this composable is actually a container for all workflow-related composables, and so I think the name is actually appropriate (closes #22). To render a workflow without a `ViewEnvironment`, `renderAsState` is now public. This is also much more idiomatic, as it resembles APIs like `Flow<T>.collectAsState` and `Observable.subscribeAsState`.
zach-klippenstein
force-pushed
the
zachklipp/renderasstate
branch
from
14148a8 to
4a5373a
Compare
|
Fixed tests, added more tests, and refactored |
rjrjr
reviewed
May 29, 2020
| * this value changes while this function is in the composition, the runtime will be restarted. | ||
| */ | ||
| @Composable | ||
| fun <PropsT, OutputT : Any, RenderingT> Workflow<PropsT, OutputT, RenderingT>.renderAsState( |
There was a problem hiding this comment.
This is so clean. It has that "this is obviously the right thing to do, why did we do anything else" feel to it.
There was a problem hiding this comment.
Yep. It's almost the exact same API as Flow/Rx subscriptions in Compose too.
|
Unclear to me what all changed since the first review this morning. Still LG. |
# for free
to subscribe to this conversation on GitHub.
Already have an account?
#.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This narrows the scope of
WorkflowContainerto be only for showing a workflow's renderingsusing a
ViewEnvironment. This elimnates the need to have separate overloads for workflowswith rendering type
ComposeRendering, and is more idiomatic – the oldWorkflowContainerwasn't a "container" in any Compose-y sense of the word, and it was really weird that it took
a content function. Now this composable is actually a container for all workflow-related
composables, and so I think the name is actually appropriate (closes #22).
To render a workflow without a
ViewEnvironment,renderAsStateis now public. This is alsomuch more idiomatic, as it resembles APIs like
Flow<T>.collectAsStateandObservable.subscribeAsState.