-
Notifications
You must be signed in to change notification settings - Fork 103
wip: Modernize tutorial final code #1316
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
base: main
Are you sure you want to change the base?
Conversation
c5676e6
to
549a127
Compare
1c3247f
to
8a04e34
Compare
// TODO: add properties needed to update WelcomeViewBinding | ||
) : AndroidScreen<WelcomeScreen> { | ||
|
||
override val viewFactory = |
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.
Should we just use compose for the tutorial as that is most likely what users will have?
> Besides `ScreenViewFactory.fromViewBinding` we also provide `ScreenViewFactory.fromCode` | ||
> (to simplify building a `View` completely by hand) and a few other varieties. | ||
> | ||
> Or feel free to give `ComposeScreen` a try instead of `AndroidScreen`. |
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.
Feel this should be the default, keeps complexity of the tutorial much lower, can be an extra topic at the end if users want to learn more about how to run with xml.
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.
Been wrestling with that but first want to focus on just bringing what exists up to date, which is a much smaller lift.
|
||
### Workflows and Rendering Type | ||
|
||
The core responsibility of a workflow is to provide a complete "rendering" every time the related state updates. Let's go into the `WelcomeWorkflow` now, and have it return a `WelcomeScreen` in the `render` method. | ||
The core responsibility of a workflow is to provide a complete rendering / view model |
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.
Might want to clarify it's only a data container a view model as provided by the androidx version. I almost feel that view model might confuse more than explain anything.
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.
Yeah, I can stress that more and use the term less in other spots. But I would like to reclaim the term a bit here. It really is exactly what they are.
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.
And note that we said "view model," not "Jetpack ViewModel."
A workflow rendering is a view model in the classic sense:
it is a struct-like bag of values and event handlers that provide just the information needed to create a view,
with no coupling to the platform specific concerns of any particular UI system.
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.
👍 great!
samples/tutorial/Tutorial1.md
Outdated
|
||
// This doesn't look like much right now, but we'll add more layout runners shortly. | ||
private val viewRegistry = ViewRegistry(WelcomeLayoutRunner) | ||
Let's also opt in to some "experimental" `runtimeConfig` options -- all of them, in fact. |
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.
Maybe we can move this section down just above the TutorialViewModel
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.
I'll make it a !NOTE
block below the code snippet -- had it that way originally actually. Don't want to break up the code though.
state = state.copy(username = username) | ||
import com.squareup.workflow1.action | ||
|
||
private fun updateName(name: String) = action("updateName") { |
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.
We name the action here, but the WorkflowAction below doesn't name it. Might want to explain what it does, or why it's missing below.
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.
I'll add the missing override val debuggingName: String
line, that's what it boils down to.
samples/tutorial/Tutorial2.md
Outdated
|
||
Modify the rendering to return a `TodoListScreen`. Modify `State` data class to contain a placeholder parameter, to make the compiler happy. We can leave everything else as the default for now: | ||
Modify `rendering()` to return a `TodoListScreen`. Modify `State` data class to contain a placeholder parameter, to make the compiler happy. We can leave everything else as the default for now: |
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.
Modify `rendering()` to return a `TodoListScreen`. Modify `State` data class to contain a placeholder parameter, to make the compiler happy. We can leave everything else as the default for now: | |
Modify `render()` to return a `TodoListScreen`. Modify `State` data class to contain a placeholder parameter, to make the compiler happy. We can leave everything else as the default for now: |
@@ -209,17 +147,17 @@ Finally, update `render` for `TodoListWorkflow` to send the titles of the todo m | |||
```kotlin | |||
object TodoListWorkflow : StatefulWorkflow<Unit, State, Nothing, TodoListScreen>() { | |||
|
|||
// … |
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.
I think it's missing the state here with the new todos
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.
I don't think so? We already declared State.todos
, above and set up a cutesy placeholder TodoModel in initialState
. Am I missing something?
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.
It's there my bad, github had hidden that line since it never changed all good!
samples/tutorial/Tutorial2.md
Outdated
|
||
```kotlin | ||
object RootWorkflow : StatefulWorkflow<Unit, State, Nothing, WelcomeScreen>() { | ||
object RootNavigationWorkflow : StatefulWorkflow<Unit, State, Nothing, WelcomeScreen>() { | ||
|
||
sealed class State { |
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.
could be sealed interface (but not a big deal)
props = TodoProps(renderState.username) | ||
) { | ||
// When TodoNavigationWorkflow emits Back, enqueue our log out action. | ||
logOut |
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.
logOut | |
logOut() |
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.
Nope:
private val logOut = action("logout") {
state = ShowingWelcome
}
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.
Oh, but that bit is wrong in the markdown.
|
||
> [!TIP] | ||
> `BackStackScreen` isn't the only navigation UI primitive that workflow provides, | ||
> (though so far it is the only one covered by this tutorial, sorry sorry sorry.) |
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.
Lol left us hanging here. What other ones are there? Or where can we link to read more about them?
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.
I'll add links to this bit:
> Look for the `Overlay` marker interface, implemented by renderings that model
> things like Android `Dialog` windows, and the `BodyAndOverlaysScreen` that
> arranges them in layers over a body `Screen`.
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.
> [!TIP]
> `BackStackScreen` isn't the only navigation UI primitive that workflow provides,
> (though so far it is the only one covered by this tutorial, sorry sorry sorry.)
> If you look in [the `navigation` package](https://github.com/square/workflow-kotlin/blob/main/workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/navigation/) you will also see:
>
> - The `Overlay` marker interface, implemented by renderings that model things like Android `Dialog` windows
> - `ScreenOverlay` for modeling an `Overlay` whose content comes from a `Screen`
> - `BodyAndOverlaysScreen`, a class that arranges `Overlay` instances in layers over a body `Screen`.
> - And `the [AndroidOverlay](https://github.com/square/workflow-kotlin/blob/main/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/AndroidOverlay.kt)` interface that simplifies implementing `ScreenOverlay` with Android's `AppCompatDialog` class.
ec2422f
to
3c32e11
Compare
Our templates were out of date, and our `install.sh` script doesn't work any more. Replaced with an IDE-produced zip file that the IDE is willing to import. Replaces `Layout Runner (ViewBinding)` with `Android Screen (ViewBinding)`: ```kotlin package ${PACKAGE_NAME} import com.squareup.workflow1.ui.AndroidScreen import com.squareup.workflow1.ui.ScreenViewFactory import com.squareup.workflow1.ui.ScreenViewRunner import com.squareup.workflow1.ui.ViewEnvironment import com.squareup.workflow1.ui.WorkflowUiExperimentalApi @OptIn(WorkflowUiExperimentalApi::class) data class $Name( // TODO: add properties needed to update $VIEW_BINDING_TYPE ) : AndroidScreen<$Name> { override val viewFactory = ScreenViewFactory.fromViewBinding($VIEW_BINDING_TYPE::inflate, ::${Name}Runner) } @OptIn(WorkflowUiExperimentalApi::class) private class ${Name}Runner( private val viewBinding: $VIEW_BINDING_TYPE ) : ScreenViewRunner<$Name> { override fun showRendering( rendering: $Name, environment: ViewEnvironment ) { TODO("Update viewBinding from rendering") } } ```
c970e8f
to
4595e6c
Compare
Long lived branch (which I will totally rebase like mad) accumulating incremental tutorial updates. - `README.md` done - `tutorial-base` done - `Tutorial1.md` done - `tutorial-1-complete` done - `Tutorial2.md` done - `tutorial-2-complete` done - `Tutorial3.md` done - `tutorial-3-complete` done - `Tutorial4.md` done - `tutorial-4-complete` done - `tutorial-final` is updated (with the above) and its tests pass It's time to get the Tutorial caught up with undeprecated API: - Explicit discussion of out big picture navigation story (so far see the end of Tutorial2.md) - Use `AndroidScreen` and drop `ViewRegistry` - Better, more consistent use of `BackStackScreen` - Use `View.setBackHandler` - Use `TextController` - Use `RequestContext.eventHandler` - Delete a lot of `// Exactly what the function name and the code say` comments - Hint at the existance of `ComposeWorkflow` without turning this into a complete rewrite - More consistent naming, code style for actions and event handlers in `Screen` renderings - Event handler fields are named `onVerbPhrase`, like `onBackPressed` - Action names are verb phrases describing the action that is being taken, not the event that is being handled - Output names are generally in terms of the semantic event being reported to the parent, rather than describing what the parent will do Thus: ```kotlin return TodoListScreen( onRowPressed = { context.actionSink.send(reportSelection(it)) } ``` ```kotlin private fun reportSelection(index: Int) = action { // Tell our parent that a todo item was selected. setOutput(TodoSelected(index)) } ``` (Although most of these will be inlined as `eventHandler("…") {}` calls.) When the rest of the `tutorial-N` modules have been updated I'll put this up for review. I've mainly focussed on updating what exists, not extending its coverage further. Once this is merged I'd like to follow up and do more: - Introduce `Overlay` (though it's mentioned). - How about move `TodoEditScreen` to a `BottomSheetDialog`? - Could be too noisy, `BottomSheetDialog` is a bastard. - If that's the case create a separate sample for it. - Introduce `Compose`. - I think the way to go is add a fifth step that updates `TodoListScreen`, talk about getting rid of that `RecyclerView`. - Should also mention at the top that people can use `ComposeScreen` instead of `AndroidScreen`; and on the `TextController` page call out `asMutableState()`
4595e6c
to
aa77bc6
Compare
Long lived branch (which I will totally rebase like mad) accumulating incremental tutorial updates.
README.md
donetutorial-base
doneTutorial1.md
donetutorial-1-complete
doneTutorial2.md
donetutorial-2-complete
doneTutorial3.md
donetutorial-3-complete
doneTutorial4.md
donetutorial-4-complete
donetutorial-final
is updated (with the above) and its tests passIt's time to get the Tutorial caught up with undeprecated API:
Explicit discussion of out big picture navigation story (so far see the end of Tutorial2.md)
Use
AndroidScreen
and dropViewRegistry
Better, more consistent use of
BackStackScreen
Use
View.setBackHandler
Use
TextController
Use
RequestContext.eventHandler
Delete a lot of
// Exactly what the function name and the code say
commentsHint at the existance of
ComposeWorkflow
without turning this into a complete rewriteMore consistent naming, code style for actions and event handlers in
Screen
renderingsonVerbPhrase
, likeonBackPressed
Thus:
(Although most of these will be inlined as
eventHandler("…") {}
calls.)When the rest of the
tutorial-N
modules have been updated I'll put this up for review.I've mainly focussed on updating what exists, not extending its coverage further. Once this is merged I'd like to follow up and do more:
Overlay
(though it's mentioned).TodoEditScreen
to aBottomSheetDialog
?BottomSheetDialog
is a bastard.Compose
.TodoListScreen
, talk about getting rid of thatRecyclerView
.ComposeScreen
instead ofAndroidScreen
;and on the
TextController
page call outasMutableState()