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 - Sign in #5995

Merged
merged 15 commits into from
May 25, 2022
Merged

FTUE - Sign in #5995

merged 15 commits into from
May 25, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented May 9, 2022

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

Uses the updated combined sign in screen for the sign in flow (a slightly tweaked version of the sign up #5648)

  • Creates dedicated Authenticate sub type for the onboarding actions along with a DirectLogin, removing the ambiguity around LoginOrRegister
  • Starts removing the duplication in the combined register and login fragments (extracting logic to classes or extensions)

Motivation and context

Fixes #5283
Forgot password and editing server selection tweaks will be done in another PR

Screenshots / GIFs

LIGHT DARK GIF ALREADY HAVE AN ACCOUNT GIF SIGN IN
Screenshot_20220509_155709 Screenshot_20220509_155655 after-sign-in after-sign-in-2

Tests

  • Enable the combined login feature flag
  • In the onboarding splash screen tap I already have an account

Tested devices

  • Physical
  • Emulator
  • OS version(s): 28

@ouchadam ouchadam added Z-FTUE Issue is relevant to the first time use project or experience PR-Large PR with more than 500 updated lines labels May 9, 2022
@ouchadam ouchadam marked this pull request as draft May 9, 2022 16:13
@@ -119,40 +120,43 @@ class FtueAuthLoginFragment @Inject constructor() : AbstractSSOFtueAuthFragment<
}

private fun submit() {
cleanupUi()
withState(viewModel) { state ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this block is an indentation change due to needing the state in order to create the AuthenticateAction from the SignMode, this isn't needed by the new combined fragments as they don't share the same fragment

@github-actions
Copy link

github-actions bot commented May 9, 2022

Unit Test Results

124 files  124 suites   2m 9s ⏱️
220 tests 220 ✔️ 0 💤 0
726 runs  726 ✔️ 0 💤 0

Results for commit 8c44c98.

♻️ This comment has been updated with latest results.

@ouchadam ouchadam changed the base branch from base/adm/sign-in-base-branch to develop May 23, 2022 10:15
@ouchadam ouchadam force-pushed the feature/adm/ftue-sign-in branch from 9a28949 to 3a5fe5f Compare May 23, 2022 13:53
@ouchadam ouchadam marked this pull request as ready for review May 23, 2022 16:07
@ouchadam ouchadam requested review from a team, onurays, ganfra and ericdecanini and removed request for a team and onurays May 23, 2022 16:07
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.

Really nice work! Just one remark regarding fragment injection

@@ -521,6 +524,21 @@ interface FragmentModule {
@FragmentKey(FtueAuthPersonalizationCompleteFragment::class)
fun bindFtueAuthPersonalizationCompleteFragment(fragment: FtueAuthPersonalizationCompleteFragment): Fragment

@Binds
Copy link
Member

Choose a reason for hiding this comment

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

Had we not decided to start using hilt for fragment too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not yet but there's a plan that I'm very excited for 😄 #5420

there's a lot of onboarding fragments

@ouchadam ouchadam force-pushed the feature/adm/ftue-sign-in branch from a0fcef6 to 8c44c98 Compare May 25, 2022 09:30
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@ericdecanini ericdecanini left a comment

Choose a reason for hiding this comment

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

Looks awesome!

@ouchadam ouchadam merged commit 0675b7c into develop May 25, 2022
@ouchadam ouchadam deleted the feature/adm/ftue-sign-in branch May 25, 2022 16:30
@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=20 failures=0 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=3 failures=0 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=58 failures=2 errors=0 skipped=3
  • [org.matrix.android.sdk.ordering]
    = passed=16 failures=0 errors=0 skipped=0
  • [org.matrix.android.sdk.PermalinkParserTest]
    = passed=2 failures=0 errors=0 skipped=0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Large PR with more than 500 updated lines 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.

Mobile FTUE: Sign in flow (Android)
3 participants