FTUE - Combined registration #5648

ouchadam · 2022-03-28T12:37:27Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Introduces a new Sign Up screen behind a feature flag, the screen aims to simplify and merge the server selection process.

The feature is behind a feature flag
The server selection edit button is disabled and will be added back in the next PR

Motivation and context

To help simplify the sign up process during the FTUE (first time user experience)

Screenshots / GIFs


TABLET LANDSCAPE
TABLET PORTRAIT
PIXEL 4 XL
SMALL

WITH SSO	WITHOUT SSO

GIF

Tests

Enable the combined sign up feature
Create a new account with username and password or SSO

Tested devices

Physical
Emulator
OS version(s): 31 Sv2

github-actions · 2022-03-28T13:30:57Z

Unit Test Results

110 files ±0 110 suites ±0 1m 12s ⏱️ -7s
195 tests ±0 195 ✔️ ±0 0 💤 ±0 0 ❌ ±0
650 runs ±0 650 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 5120e7a. ± Comparison against base commit 7067c50.

♻️ This comment has been updated with latest results.

amshakal

It's looking great. As discussed, eventually we will have an edit button added to this screen for the user to change their server.

TY!

mnaturel

Just small remarks mainly about naming.

mnaturel · 2022-03-31T13:33:35Z

vector/src/debug/java/im/vector/app/features/debug/features/DebugVectorFeatures.kt

    val onboardingSplashCarousel = booleanPreferencesKey("onboarding-splash-carousel")
    val onboardingUseCase = booleanPreferencesKey("onbboarding-splash-carousel")
    val onboardingPersonalize = booleanPreferencesKey("onbboarding-personalize")
+    val onboardingCombinedChooseServer = booleanPreferencesKey("onbboarding-combined-choose-server")


Is it intended on some keys the "b" of "onboarding" is doubled?

nope... and then it was copy pasted 🤦

mnaturel · 2022-03-31T13:38:51Z

vector/src/main/java/im/vector/app/core/extensions/ConstraintLayout.kt

    }
 }
+
+fun ConstraintLayout.realignPercentagesToParent() {


Perhaps it would be interesting to have a comment explaining the purpose of the method? If my understanding is correct it only updates the children views according to the height but there is no mention of that in the function name. Do you think the current naming is sufficient?

great point, will do! the tl;dr is that constraint layouts within scrollviews use the total scrolling height rather than the viewport, which means percentages change size depending on how much they can scroll

mnaturel · 2022-03-31T13:51:22Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewEvents.kt


    object OpenUseCaseSelection : OnboardingViewEvents()
    object OpenServerSelection : OnboardingViewEvents()
+    object OpenCombinedServerSelection : OnboardingViewEvents()


Maybe it would be better to use the same naming convention everywhere to be consistent? I see sometimes, we talk about combinedServer or combinedChooseServer or combinedSignUp. My vote would go for combinedSignUp.

👍 will check in with FTUE team to see if they have a unique name for this screen (and the server selection) that avoid conflicting with the previous versions

CombinedRegister seems like a reasonable intermediate name

801fb90 (and updates the tag)

mnaturel · 2022-03-31T13:57:16Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingAction.kt


    // Login or Register, depending on the signMode
    data class LoginOrRegister(val username: String, val password: String, val initialDeviceName: String) : OnboardingAction
+    data class Register(val username: String, val password: String, val initialDeviceName: String) : OnboardingAction


Concerning the naming, I think we should stick to either register or signUp to be consistent accross the app. What do you think?

100% agree, would favour using register & login.

For now I'll rename the code introduced by the PR rather than renaming the entire onboarding package as it will cause a lot of noise in the diff

mnaturel · 2022-03-31T15:50:42Z

...r/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthCombinedSignUpFragment.kt

+                error++
+            }
+            if (state.isNumericOnlyUserIdForbidden() && login.isDigitsOnly()) {
+                views.createAccountInput.error = "The homeserver does not accept username with only digits."


Perhaps we should declare this string into the resources?

💯 will also extract the source https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthLoginFragment.kt#L138

mnaturel · 2022-03-31T15:56:15Z

vector/src/main/java/im/vector/app/features/onboarding/ftueauth/FtueAuthVariant.kt

+    private fun openCombinedSelectionSignUp() {
+        activity.addFragmentToBackstack(
+                views.loginFragmentContainer,
+                FtueAuthCombinedSignUpFragment::class.java,


Is there a need for a tag: FRAGMENT_REGISTRATION_STAGE_TAG ? I see it is used for all other registration fragments.

it's being used to selectively control the backstack, it's not technically needed for this screen but will add for consistency 👍

…screen

…t layout - this allows percentages to be used which make of the screen viewport rather than the accumulated scroll height

… and providing a dedicated register action

…ken into account

- we rely on the text size and padding instead

…ut the server selection screen

- also ports the Sso listener to a functional interface

- also adds a missing tag to the fragment adding

mnaturel · 2022-04-01T07:33:22Z

vector/src/main/java/im/vector/app/features/VectorFeatures.kt

    override fun isOnboardingSplashCarouselEnabled() = true
    override fun isOnboardingUseCaseEnabled() = true
    override fun isOnboardingPersonalizeEnabled() = false
+    override fun isOnboardingCombinedChooseServerEnabled() = false


We could also rename the feature flag as isOnboardingCombinedRegisterEnabled(). What do you think?

agreed 👍 aac206f

Thanks! I see a last thing to rename, the boolean inside the Factory:

createBooleanFeature( label = "FTUE Combined register", key = DebugFeatureKeys.onboardingCombinedRegister, factory = VectorFeatures::isOnboardingCombinedRegisterEnabled )

I should have done a search replace 😅 amended the commit above ^^^ 30635af

Was it intented to not rename the key DebugFeatureKeys.onboardingCombinedChooseServer? Sorry to annoy you with namings...

this is my bad! third time lucky 🤞 5120e7a

mnaturel

Good for me to merge.

ouchadam added the X-Needs-Design May require input from the design team label Mar 28, 2022

ouchadam requested a review from amshakal March 29, 2022 08:20

ouchadam force-pushed the feature/adm/enabling-personalisation branch from d1bed4b to 0ffd20e Compare March 29, 2022 16:48

Base automatically changed from feature/adm/enabling-personalisation to develop March 30, 2022 08:09

ouchadam force-pushed the feature/adm/ftue-sign-up branch from 62044f3 to fcb7c73 Compare March 30, 2022 08:16

amshakal approved these changes Mar 30, 2022

View reviewed changes

ouchadam force-pushed the feature/adm/ftue-sign-up branch from fcb7c73 to 9c044fb Compare March 30, 2022 12:52

ouchadam marked this pull request as ready for review March 30, 2022 12:54

ouchadam requested review from a team and mnaturel and removed request for a team March 30, 2022 12:56

ouchadam removed the X-Needs-Design May require input from the design team label Mar 31, 2022

mnaturel reviewed Mar 31, 2022

View reviewed changes

ouchadam added 16 commits March 31, 2022 17:50

adding debug flag for enabling the new combined signup/choose server …

b439322

…screen

adding helper for recalculating percentage heights within a constrain…

2cbbfca

…t layout - this allows percentages to be used which make of the screen viewport rather than the accumulated scroll height

adding combined server selection/sign up fragment

7e5c3df

replacing hardcoded strings with resources

1198344

extracting common textinputlayer interactions to their own extensions…

585ac4b

… and providing a dedicated register action

adding missing inputType, fixes max lines and ime option not being ta…

c3ab895

…ken into account

renaming reset action to also capture registering along with login

e8791fb

reducing the edit button min width in order to match designs

53675b5

- we rely on the text size and padding instead

temporarily hiding the server selection edit button whilst building o…

bc4566d

…ut the server selection screen

reordering methods to match usage

12ae35f

- also ports the Sso listener to a functional interface

providing more context to screen opening function

d302875

updating debug switch copy to better reflect the feature

c83882d

defaulting the password field to the password type by default in xml

aa5054c

using continuation copy for combined sign up SSO buttons

32b54e1

adding changelog entry

4e215fa

fixing new file header date

4c1c9a5

ouchadam added 4 commits March 31, 2022 17:50

fixing type in debug key names

468a81e

adding docs around the realigning of constraint layout child percentages

c45c421

using a consistent name for the combined register screens and events

801fb90

- also adds a missing tag to the fragment adding

extracting hardcoded error message to the resources

13fb4e5

ouchadam force-pushed the feature/adm/ftue-sign-up branch from ac88f9a to 13fb4e5 Compare March 31, 2022 17:07

mnaturel reviewed Apr 1, 2022

View reviewed changes

ouchadam force-pushed the feature/adm/ftue-sign-up branch from aac206f to 30635af Compare April 1, 2022 09:48

updating feature flag name to match feature name in other places

5120e7a

ouchadam force-pushed the feature/adm/ftue-sign-up branch from 30635af to 5120e7a Compare April 1, 2022 12:13

mnaturel approved these changes Apr 1, 2022

View reviewed changes

ouchadam merged commit 27b727e into develop Apr 1, 2022

ouchadam deleted the feature/adm/ftue-sign-up branch April 1, 2022 12:51

ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Apr 1, 2022

ouchadam changed the title ~~FTUE - Combined sign up + server selection screen~~ FTUE - Combined registration Apr 5, 2022

ouchadam mentioned this pull request Apr 5, 2022

FTUE - Server selection #5691

Merged

6 tasks

ouchadam mentioned this pull request May 9, 2022

FTUE - Sign in #5995

Merged

6 tasks

FTUE - Combined registration #5648

FTUE - Combined registration #5648

Uh oh!

Conversation

ouchadam commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Uh oh!

github-actions bot commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

amshakal left a comment

Choose a reason for hiding this comment

Uh oh!

mnaturel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaturel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ouchadam commented Mar 28, 2022 •

edited

Loading

github-actions bot commented Mar 28, 2022 •

edited

Loading

ouchadam Mar 31, 2022 •

edited

Loading