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.

Sign up for GitHub

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

Jump to bottom

FTUE - Single loading state #5517

Merged
merged 8 commits into from
Mar 24, 2022
Merged

FTUE - Single loading state #5517

merged 8 commits into from
Mar 24, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Mar 11, 2022 •
edited
Loading

Marked as draft as this relies on #5408

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Flattens the onboarding async states down into a single isLoading
  • Uses a single flow for error handling instead of splitting them across state and events, all errors flow through the events, it's up to the individual fragments to explicitly opt into custom error handling

Motivation and context

To simplify, remove code duplication and avoid redundant state updates

Screenshots / GIFs

GENERIC INVALID USERNAME IN USE
o-no-network o-invalid-username Screenshot_20220311_131849

Tests

  • In the sign up page
  • cause an error
    • Disconnect from the internet and attempt to create an account
    • Use an invalid account name
    • Attempt to use an existing account
    • Type a password with more than 512 characters

Tested devices

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

@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Mar 11, 2022
@ouchadam ouchadam marked this pull request as draft March 11, 2022 13:31
@github-actions
Copy link

github-actions bot commented Mar 11, 2022 •
edited
Loading

Unit Test Results

106 files  ±0  106 suites  ±0   1m 10s ⏱️ -5s
188 tests ±0  188 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
622 runs  ±0  622 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit af90ada. ± Comparison against base commit adf2c64.

♻️ This comment has been updated with latest results.

This was referenced Mar 11, 2022
Merged
Closed
@ouchadam ouchadam force-pushed the feature/adm/onboarding-tests branch from 7a4e4a2 to 3d20d46 Compare March 15, 2022 17:52
Base automatically changed from feature/adm/onboarding-tests to develop March 18, 2022 15:57
@ouchadam ouchadam force-pushed the feature/adm/ftue-single-loading-state branch from 63c7e2b to bbc892f Compare March 18, 2022 16:35
@ouchadam ouchadam marked this pull request as ready for review March 18, 2022 16:36
ouchadam
ouchadam commented Mar 18, 2022
View reviewed changes
vector/src/main/java/im/vector/app/features/onboarding/ftueauth/AbstractFtueAuthFragment.kt
@@ -86,21 +84,8 @@ abstract class AbstractFtueAuthFragment<VB : ViewBinding> : VectorBaseFragment<V
is CancellationException ->
/* Ignore this error, user has cancelled the action */
Unit
is Failure.ServerError ->
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 has been lifted to the Login/Sign up fragment https://github.com/vector-im/element-android/pull/5517/files#diff-6422bfd992cde38d5783f859483c70b55ed045bc2c89a153784957ff2d0550e0R277 (the same change was made in Login2)

@ouchadam ouchadam requested review from a team, ahmed-radhouane and onurays and removed request for a team and ahmed-radhouane March 21, 2022 09:18
onurays
onurays approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@onurays onurays left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice cleanup! Statically reviewed, all LGTM. (Please see conflicted files before merging)

ouchadam added 8 commits March 23, 2022 16:48
@ouchadam ouchadam force-pushed the feature/adm/ftue-single-loading-state branch from bbc892f to af90ada Compare March 23, 2022 17:29
@ouchadam
Copy link
Contributor Author

ouchadam commented Mar 23, 2022 •
edited
Loading

@onurays thanks for the review! I have rebased on develop, will do another sanity check then merge if the CI is still happy 🤞

@ouchadam ouchadam merged commit 84b34f7 into develop Mar 24, 2022
@ouchadam ouchadam deleted the feature/adm/ftue-single-loading-state branch March 24, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onurays onurays onurays approved these changes

Assignees
No one assigned
Labels
Z-FTUE Issue is relevant to the first time use project or experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ouchadam @onurays