-
Notifications
You must be signed in to change notification settings - Fork 739
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
FTUE - Account created screen #5158
Conversation
Matrix SDKIntegration Tests Results:
|
…tate - allows for greater control of the flow (such as adding new screens inbetween the creation and exit with flags)
…ps are still TODO
… into the main app
…avoid going back to the sign up screens
8b9d484
to
c308142
Compare
c308142
to
e0f99e3
Compare
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.
Thanks, LGTM, just one remark!
@@ -147,6 +147,8 @@ class OnboardingViewModel @AssistedInject constructor( | |||
is OnboardingAction.UserAcceptCertificate -> handleUserAcceptCertificate(action) | |||
OnboardingAction.ClearHomeServerHistory -> handleClearHomeServerHistory() | |||
is OnboardingAction.PostViewEvent -> _viewEvents.post(action.viewEvent) | |||
OnboardingAction.PersonalizeProfile -> _viewEvents.post(OnboardingViewEvents.OnPersonalizeProfile) |
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.
Do we really need that? It's ok but feels a bit useless when there is really no other code involved
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 wasn't sure about this as well, sounds like I should use OnboardingAction.PostViewEvent
which acts as a proxy to post events directly to the ViewModel.viewEvents
observer
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.
Thanks!
…ed actions/events
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.
Small small remarks. Please merge once handled. Thanks!
|
||
private fun setupViews() { | ||
views.accountCreatedSubtitle.text = getString(R.string.ftue_account_created_subtitle, activeSessionHolder.getActiveSession().myUserId) | ||
views.accountCreatedPersonalize.setOnClickListener { viewModel.handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnPersonalizeProfile)) } |
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.
Nit: use debouncedClicks
instead of setOnClickListener
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.
👍 updated 182dc2a
private fun setupViews() { | ||
views.accountCreatedSubtitle.text = getString(R.string.ftue_account_created_subtitle, activeSessionHolder.getActiveSession().myUserId) | ||
views.accountCreatedPersonalize.setOnClickListener { viewModel.handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnPersonalizeProfile)) } | ||
views.accountCreatedTakeMeHome.setOnClickListener { viewModel.handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnTakeMeHome)) } |
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.
Same
|
||
override fun onBackPressed(toolbarButton: Boolean): Boolean { | ||
viewModel.handle(OnboardingAction.PostViewEvent(OnboardingViewEvents.OnTakeMeHome)) | ||
return 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.
I think you should return true
here, but the contract is not clear (too me). So I am not sure. But it looks like you consume the back event so you should stop the propagation (if I am right).
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.
ah great catch! 4975406
@@ -16,4 +16,10 @@ | |||
|
|||
<!-- onboarding english only word play --> | |||
<string name="cut_the_slack_from_teams" translatable="false">Cut the slack from teams.</string> | |||
|
|||
<!-- WIP --> | |||
<string name="ftue_account_created_personalize" translatable="false">Personalise profile</string> |
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.
Default is EN-US so it has to be
<string name="ftue_account_created_personalize" translatable="false">Personalise profile</string> | |
<string name="ftue_account_created_personalize" translatable="false">Personalize profile</string> |
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.
👍 will also let design/product know 06f2ca1
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.
Thanks for the update!
Part of #4586 element-hq/element-meta#132
personalise profile
button TODO is handled by FTUE - Choose a display name #5211