Modernize tutorial final code #1116
Conversation
rjrjr
force-pushed
the
ray/fresh-tutorials
branch
4 times, most recently
from
684b931 to
fe04b90
Compare
rjrjr
force-pushed
the
ray/fresh-tutorials
branch
2 times, most recently
from
6b66f94 to
36bcfe6
Compare
| } | ||
|
|
||
| private fun onDiscard() = action { | ||
| private val postDiscard = action { |
There was a problem hiding this comment.
nit: not understanding where the 'post' namign is coming from? I think i get that we are 'posting' either a discard or a save to our parent Workflow? Is that the intention?
I'm not sure that's helpful to me though as the name of the action.
LIke is the action about what it does (posting a save) or what it responds to (on "Save" clicked)? I feel like we've often used the latter, so maybe that's what is tripping me up. Maybe the former is preferrable?
Maybe this is all pedantic (though if there's any place to be so, it would be the tutorial...).
I, for one, long for Action suffixes when we save actions like this. As in could we use discardAction, saveAction -> telling you what it is (Action) and what it does (discard or save) but not the implementation details ('posting' to the parent).
There was a problem hiding this comment.
LIke is the action about what it does (posting a save) or what it responds to (on "Save" clicked)? I feel like we've often used the latter, so maybe that's what is tripping me up. Maybe the former is preferrable?
I like the *Action notion, will adopt.
There was a problem hiding this comment.
Actually…
What I liked about the awkward post* names was that they communicated what was going to happen in response to the UI event. I don't think that's an implementation detail, it helps make visible what the role of this workflow is in the overall structure. And once I've put the names in that very active format, the *Action suffix starts to sound noisy.
WDYT:
onTodoSelected = { context.actionSink.send(requestSelection(it)) },
onBackPressed = { context.actionSink.send(requestGoBack) },
onAddPressed = { context.actionSink.send(requestNew) }
samples/tutorial/tutorial-final/src/main/java/workflow/tutorial/TodoListScreen.kt
Outdated
Show resolved
Hide resolved
| ) | ||
| } | ||
|
|
||
| private fun onBack() = action { | ||
| private val postGoBack = action { |
| title: String, | ||
| note: String | ||
| ) : this(TextController(title), TextController(note)) | ||
| } |
There was a problem hiding this comment.
is this used directly in the rendering?
There was a problem hiding this comment.
Yes. That's the normal pattern for TextController, which is why I've pushed back when anyone talks about generalizing it.
...ow-ui/container-common/src/main/java/com/squareup/workflow1/ui/modal/AlertContainerScreen.kt
Outdated
Show resolved
Hide resolved
rjrjr
force-pushed
the
ray/fresh-tutorials
branch
from
36bcfe6 to
506d00f
Compare
It's time to get the Tutorial caught up with undeprecated API. This PR modifies only the final tutorial module. Once we're happy with the code, I'll back port the changes and update the prose in follow ups.
- Use `AndroidScreen` and drop `ViewRegistry`
- Use `View.setBackHandler`
- Use `TextController`
- More consistent naming, code style for actions and event handlers in `Screen` renderings
I did not introduce `RenderContext.eventHandler {}`, that seems like it would just be confusing to a newcomer.
Also not introducing big new blocks of material, in particular not introducing `Overlay`. I do think we should do that, but for this release I just want to focus on getting the deprecated code deleted.
rjrjr
force-pushed
the
ray/fresh-tutorials
branch
from
8ff126f to
79a66f9
Compare
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There are a lot of indentation changes here, good idea to hide whitespace while reviewing.
It's time to get the Tutorial caught up with undeprecated API. This PR modifies only the final tutorial module. Once we're happy with the code, I'll back port the changes and update the prose in follow ups.
AndroidScreenand dropViewRegistryView.setBackHandlerTextControllerScreenrenderingsI did not introduce
RenderContext.eventHandler {}, that seems like it would just be confusing to a newcomer.Also not introducing big new blocks of material, in particular not introducing
Overlay. I do think we should do that, but for this release I just want to focus on getting the deprecated code deleted.