-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add performAction function to ReEngagePrompt #2078
Conversation
a2e1d6a
to
4341869
Compare
|
||
if (state == ReEngageCustomPromptDemoScreenState.Idle) { | ||
SideEffect { | ||
viewModel.initialize() |
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 understand this is correct and our guidance, it does create a lot of visual noise.
Do we know why apps like Now in Android don't follow the guidance?
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 know, what about asking in https://github.com/android/nowinandroid/discussions ?
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.
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.
The answer seems to be in DAC:
Flow APIs allow for this with the started argument in the stateIn method.
Which seems to be what NiA is using.
Then it continues:
In the cases where this is inapplicable, define an idempotent initialize() function to explicitly start the state production pipeline as shown in the following snippet:
So we should re-evaluate our cases and see when the stateIn + started WhileSubscribed is applicable.
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.
private val reEngagePrompt: ReEngagePrompt, | ||
) : ViewModel() { | ||
|
||
private var initializeCalled = false |
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.
For a follow up. Are there ways to reduce the repetition of this initialise phase?
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 is the repetition:
Composable
SideEffect {
viewModel.initialize()
}
ViewModel
private var initializeCalled = false
@MainThread
fun initialize() {
if (initializeCalled) return
initializeCalled = true
// pipeline initialization
}
The repetition could be reduced having a base class for VM, but that's not scalable. I don't see many lines saved if composition approach is taken instead of inheritance.
For the composable, there is only 3 lines, that could be inlined.
Any thoughts?
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.
Nothing for us. If the guidance is good, the library should build this in.
But it seems very optional.
WHAT
Add
performAction
function toReEngagePrompt
.Add sample.
WHY
So it can be used with custom prompts.
Checklist 📋