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 - Combined register copy review #6545

Merged
merged 13 commits into from
Jul 19, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Jul 13, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Matches iOS and checks for user name availability on the loss of focus on the user name field
  • Updates the register page design to match the latest iteration of the designs (removes the subtitle and selected homeserver description)
  • Adds a footer to the user name reinforcing the full matrix id as a way of discoverability
  • Adds a check for minimum 8 characters password length

Motivation and context

Fixes #6546

To simplify the registration screen copy and reinforce the matrix id concept

Screenshots / GIFs

BEFORE AFTER
before-register after-register
PASSWORD MINIMUM USERNAME TAKEN FULL MATRIX ID
after-password-min after-username-taken after-full-matrix-id

Tests

  • With the combined register feature flag enabled and start the registration process

  • Attempt to register with an account that already exists

  • Attempt to register with a password shorter than 8 characters

Tested devices

  • Physical
  • Emulator
  • OS version(s):

@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Jul 13, 2022
@ouchadam ouchadam marked this pull request as ready for review July 14, 2022 09:56
@ouchadam ouchadam requested review from a team and Florian14 and removed request for a team July 14, 2022 09:56
@ouchadam ouchadam mentioned this pull request Jul 14, 2022
19 tasks
@ouchadam ouchadam force-pushed the feature/adm/ftue-combined-register-copy-review branch from 9751017 to 67676a4 Compare July 15, 2022 10:34
@ouchadam ouchadam mentioned this pull request Jul 15, 2022
6 tasks
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

53.8% 53.8% Coverage
0.0% 0.0% Duplication

import javax.inject.Inject

private const val MINIMUM_PASSWORD_LENGTH = 8
Copy link
Member

Choose a reason for hiding this comment

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

My 2 cents: some homeservers may have other rules for password (length, char type usage, etc.). We may have a first error with "password must be 8 chars minimum", then later "password must be 10 chars minimum", which is quite annoying. I would avoid clients to do local check on this field and let the config server side.

}
}

private fun maybeUpdateHomeserver(userNameOrMatrixId: String, continuation: suspend (String) -> Unit = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that the lint will be ok but I suggest to name the string parameter inside the lambda for clarity

Suggested change
private fun maybeUpdateHomeserver(userNameOrMatrixId: String, continuation: suspend (String) -> Unit = {}) {
private fun maybeUpdateHomeserver(userNameOrMatrixId: String, continuation: suspend (userName: String) -> Unit = {}) {

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this remark, also having an optional lambda for continuation is a bit strange to me.
Adam may iterate later, we need to merge the PR and review the next one before tomorrow, and this is not a blocker.

@bmarty bmarty merged commit 70c8703 into develop Jul 19, 2022
@bmarty bmarty deleted the feature/adm/ftue-combined-register-copy-review branch July 19, 2022 12:44
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.

FTUE - Register screen copy review
3 participants