feat(Onboarding): Create Profile & Login flows #16722
Open
+6,613
−511
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (545)
|
caybro
force-pushed
the
feat/onboarding-rework-1
branch
26 times, most recently
from
fe0f177 to
315bbc0
Compare
caybro
force-pushed
the
feat/onboarding-rework-1
branch
2 times, most recently
from
ad67a76 to
35e9ad5
Compare
caybro
force-pushed
the
feat/onboarding-rework-1
branch
2 times, most recently
from
51c8953 to
b312ccd
Compare
glitchminer
approved these changes
Dec 2, 2024
micieslak
reviewed
Dec 2, 2024
There was a problem hiding this comment.
It's really huge amount of good work, definitely.
I like the simplicity of particular pages, which are well-formed pure-UI components.
What I'd like to consider more in terms of app architecture are parts like OnboardingLayout. The new one is already big (over 500 loc, much bigger then the old one). But not size itself is a problem.
The OnboardingLayout depends on several stores. Stores are intended to be just a thin wrapper over methods/properties exposed from the backend. They represent backend, not needs of any particular part of the UI.
According to that, OnboardingLayout (similarly as other *Layout components in our app), depends on backend, represented by concrete types of stores. It's a violation of DIP - dependency inversion principle. OnboardingLayout depending on stores, in fact depends on concrete backend.
Depending on stores at this level also seem to violate ISP - interface segregation principle. By depending on store we provide (often) much more than is needed by given UI component.
Finally, probably most importantly, in OnboardingLayout and other components of that kind we violate single responsibility principle, leading to messy code. Those components have two responsibilityes:
- UI responsibility - arranges sub-components, controls visual flow
- Responsibility of integration of visual components with the backend via stores
In other words, we have two reasons to change that component:
- UI design (Figma) changes
- Backend changes
I think we could split those two things making big progress in terms of the code design/structure.
For the second responsibility (integration with backend) we could try to follow/adapt approach similar as the one introduced in https://github.com/status-im/status-desktop/pull/16718/files#diff-0b9f2d5f0dfacb3cedd70304e8587b2373a612e9bd61f7c9696bc67b2fb3c0f2 where the handler is non-visual component with single responsibility of configuring given visual component using some stores (ideally it should not pass stores further to SendModal at all).
The bit more problematic may be exposing in abstract way the needs of OnboardingLayout regarding interactions with the external world because, unfortunately Qml doesn't allow to define interfaces... So we cannot declare dependency on an interface type with some abstract methods. However in most cases it's nicely doable via just signals, properties with functions and regular properties.
@caybro, @alexjba, I don't have ready to use solution, but the problems here seem to be clear. I think we could propose here sth much better than that. And it's a nice chance to do that along with the new onboarding.
| KeycardBasePage { | ||
| id: root | ||
|
|
||
| signal keycardPinCreated(string pin) |
There was a problem hiding this comment.
When the user provides pin correctly (2 times, it matches), the signal is emitted as expected. However in the handler the actual keycard interaction is going to happen, and depending if it succeeds or not we should transit to the last state Keycard PIN set with green glow. KeycardCreatePinPage shouldn't go to that state automatically as it cannot know if the operation succeeded on the physical device.
So the API probably should expose dedicated method/property to allow performing that transition from outside.
There was a problem hiding this comment.
Yeah, but we will most likely delay all the interaction (with HW too) to the very last step, so for the UI component, it doesn't matter actually (and it shouldn't imo).
| horizontalAlignment: Text.AlignHCenter | ||
| } | ||
|
|
||
| SyncingEnterCode { |
There was a problem hiding this comment.
Not matter of this pr but maybe worth no note somewhere that after switching to "Enter code" active camera is no longer desired imo.
There was a problem hiding this comment.
Yeah, worth filing a separate issue maybe; just reusing the existing component here
caybro
force-pushed
the
feat/onboarding-rework-1
branch
10 times, most recently
from
fef86b4 to
132c783
Compare
caybro
force-pushed
the
feat/onboarding-rework-1
branch
from
efb0078 to
cf9b44d
Compare
caybro
force-pushed
the
feat/onboarding-rework-1
branch
3 times, most recently
from
50e907b to
917d074
Compare
jrainville
reviewed
Jan 6, 2025
ui/StatusQ/src/StatusQ/Controls/StatusPasswordStrengthIndicator.qml
Outdated
Show resolved
Hide resolved
ui/StatusQ/src/StatusQ/Controls/StatusPasswordStrengthIndicator.qml
Outdated
Show resolved
Hide resolved
| @@ -75,6 +74,11 @@ Item { | |||
| input text. | |||
| */ | |||
| property ListModel filteredList: ListModel { } | |||
|
|
|||
| property bool isError | |||
There was a problem hiding this comment.
Should this be hasErrror instead?
There was a problem hiding this comment.
Yeah maybe, just followed what we have elsewhere 🤷
| } | ||
| StatusBaseText { | ||
| Layout.fillWidth: true | ||
| text: qsTr("You will require your Keycard to log in to Status and sign transactions") |
There was a problem hiding this comment.
This sentence sounds a bit weird. I'm not sure what we're trying to tell the user
There was a problem hiding this comment.
Not sure either, pre-existing code 🤷
| ListItemButton { | ||
| objectName: "btnBySyncing" | ||
| Layout.fillWidth: true | ||
| text: qsTr("Log in by syncing") // FIXME wording, "Log in by pairing"? |
There was a problem hiding this comment.
Did we confirm if we want Pairing instead? cc @benjthayer
I personally like it better
There was a problem hiding this comment.
Yeah, not sure now; I added a FIXME there, this needs a systemic approach
| @@ -187,7 +187,7 @@ SettingsContentBase { | |||
| anchors.left: parent.right | |||
| anchors.leftMargin: 8 | |||
| anchors.verticalCenter: parent.verticalCenter | |||
| tooltipText: qsTr("Connection problems can happen.<br>If they do, please use the Enter a Seed Phrase feature instead.") | |||
| tooltipText: qsTr("Connection problems can happen.<br>If they do, please use the Enter a Recovery Phrase feature instead.") | |||
There was a problem hiding this comment.
Everywhere else, we don,t capitalize. I assume the old copy was just a mistake
jrainville
force-pushed
the
feat/onboarding-rework-1
branch
from
e505b50 to
adb3474
Compare
micieslak
approved these changes
Jan 7, 2025
- implement the basic Onboarding UI skeleton and the Create Profile flows - adjust the PasswordView and EnterSeedPhrase views to the latest design - add the main OnboardingLayout and StatusPinInput pages to Storybook - change terminology app-wide: "Seed phrase" -> "Recovery phrase" - implement the Login flows (seed, sync, keycard) - amend the keycard flow sequences with separate (non) empty page Fixes #16719 Fixes #16742 Fixes #16743
- extend the tests to verify whether we collected the correct data - restore the "pointing hand" cursor on clickable elements - some minor improvements
- instead of the factory reset when the keycard is locked
- they are not very useful (and were outdated anyway)
caybro
force-pushed
the
feat/onboarding-rework-1
branch
from
45b7c0c to
5a6613a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the PR do
Create profile UI flows:
Fixes #16719
Fixes #16742
Fixes #16743
Login UI flows:
Fixes #16798
Fixes #16799
Fixes #16800
Fixes #16977
Affected areas
Onboarding
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)