Skip to content
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

Merged
merged 13 commits into from
Feb 23, 2022
Merged

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Feb 4, 2022

Part of #4586 element-hq/element-meta#132

SCREEN FEATURE FLAG SYSTEM BACK TAKE ME HOME
Screenshot_20220204_171024 Screenshot_20220204_172701 after-system-back after-take-me-home

@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Feb 4, 2022
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

Unit Test Results

  84 files  +  8    84 suites  +8   51s ⏱️ -5s
157 tests +13  157 ✔️ +13  0 💤 ±0  0 ±0 
504 runs  +52  504 ✔️ +52  0 💤 ±0  0 ±0 

Results for commit 4975406. ± Comparison against base commit ed78e54.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed=
  • [org.matrix.android.sdk.account]
    passed=
  • [org.matrix.android.sdk.internal]
    passed=
  • [org.matrix.android.sdk.ordering]
    passed=
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed=

@ouchadam ouchadam force-pushed the feature/adm/ftue-post-account-creation branch from 8b9d484 to c308142 Compare February 7, 2022 09:24
Copy link
Member

@ganfra ganfra left a 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)
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

@bmarty bmarty left a 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)) }
Copy link
Member

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

Copy link
Contributor Author

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)) }
Copy link
Member

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
Copy link
Member

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).

Copy link
Contributor Author

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>
Copy link
Member

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

Suggested change
<string name="ftue_account_created_personalize" translatable="false">Personalise profile</string>
<string name="ftue_account_created_personalize" translatable="false">Personalize profile</string>

Copy link
Contributor Author

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

Copy link
Member

@bmarty bmarty left a 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!

@bmarty bmarty merged commit 276c526 into develop Feb 23, 2022
@bmarty bmarty deleted the feature/adm/ftue-post-account-creation branch February 23, 2022 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-FTUE Issue is relevant to the first time use project or experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants