feat(Onboarding): implement the new UnblockWithPukFlow
#17111
+750
−100
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
caybro
changed the title
UnlockWithPukFlow
caybro
force-pushed
the
feat/unlock-with-puk-flow
branch
from
8007bd9 to
159544b
Compare
Jenkins BuildsClick to see older builds (60)
|
caybro
force-pushed
the
feat/unlock-with-puk-flow
branch
2 times, most recently
from
074f0ce to
d189486
Compare
caybro
force-pushed
the
feat/unlock-with-puk-flow
branch
from
d189486 to
584ffc7
Compare
micieslak
reviewed
Jan 22, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
2 times, most recently
from
435e210 to
0cdbe5b
Compare
caybro
force-pushed
the
feat/unlock-with-puk-flow
branch
3 times, most recently
from
0de8670 to
e7aac8f
Compare
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
0cdbe5b to
40e8411
Compare
caybro
force-pushed
the
feat/unlock-with-puk-flow
branch
from
e7aac8f to
1570a45
Compare
caybro
changed the title
UnlockWithPukFlow UnblockWithPukFlow
6 tasks
caybro
requested review from
micieslak and
jrainville
and removed request for
a team
jrainville
approved these changes
Jan 23, 2025
| @@ -59,11 +60,13 @@ SQUtils.QObject { | |||
| KeycardIntroPage { | |||
| keycardState: root.keycardState | |||
| displayPromoBanner: root.displayKeycardPromoBanner | |||
| unlockUsingSeedphrase: true | |||
| unblockUsingSeedphraseAvailable: true | |||
| unblockWithPukAvailable: root.remainingAttempts === 1 || root.remainingAttempts === 2 | |||
There was a problem hiding this comment.
The remaining attempts starts at 3. I just want to confirm that the purpose of the property is really to activate after the first failure?
| @@ -37,6 +38,10 @@ QtObject { | |||
| return d.onboardingModuleInst.setPin(pin) | |||
| } | |||
|
|
|||
| function setPuk(puk: string) { // -> bool | |||
| return d.onboardingModuleInst.setPuk(puk) | |||
There was a problem hiding this comment.
FYI, I'm changing a lot of those functions in my own branch because the keycard functions are better used async since they can take a second and they would freeze the UI.
Just letting you know for the future or if you want to change it already for this one.
There was a problem hiding this comment.
OK then we'll have to introduce all sorts of "loading" states to the UI, whenever we're using the setPin() to validate the user entry
caybro
force-pushed
the
feat/unlock-with-puk-flow
branch
2 times, most recently
from
a0e0496 to
293f323
Compare
- make a difference between "Create profile..." and "Login with..." Fixes: #17109
- to be used in the "Unlock with PUK" flow
- integrate the PUK unblock flow into the Onboarding and Login screen - added a dedicated SB page for it - remove the `Locked` keycard state everywhere in favor of `BlockedPIN` and `BlockedPUK` - fix the various "Locked" buttons, based on the context and the state of the keycard Fixes: #17092
caybro
force-pushed
the
feat/unlock-with-puk-flow
branch
from
293f323 to
1d374aa
Compare
micieslak
reviewed
Jan 24, 2025
| @@ -26,6 +26,7 @@ SQUtils.QObject { | |||
| signal seedphraseSubmitted(string seedphrase) | |||
| signal reloadKeycardRequested | |||
| signal keycardFactoryResetRequested | |||
| signal unblockWithPukRequested() | |||
There was a problem hiding this comment.
cosmetics: would be nice to stick to one version regarding signals without params. In the official doc they skip (): https://doc.qt.io/qt-6/qml-codingconventions.html so I'm voting for that shorter option :)
Comment on lines
+44
to
+48
| // https://www.figma.com/design/Lw4nPYQcZOPOwTgETiiIYo/Desktop-Onboarding-Redesign?node-id=1281-45942&node-type=frame&m=dev | ||
| // https://www.figma.com/design/Lw4nPYQcZOPOwTgETiiIYo/Desktop-Onboarding-Redesign?node-id=1281-45950&node-type=frame&m=dev | ||
| // https://www.figma.com/design/Lw4nPYQcZOPOwTgETiiIYo/Desktop-Onboarding-Redesign?node-id=1281-45959&node-type=frame&m=dev | ||
| // https://www.figma.com/design/Lw4nPYQcZOPOwTgETiiIYo/Desktop-Onboarding-Redesign?node-id=1281-45966&node-type=frame&m=dev | ||
| // https://www.figma.com/design/Lw4nPYQcZOPOwTgETiiIYo/Desktop-Onboarding-Redesign?node-id=1281-45996&node-type=frame&m=dev |
There was a problem hiding this comment.
those links seem to lead to improper location
micieslak
reviewed
Jan 24, 2025
There was a problem hiding this comment.
Small glitch in pin input:
Screencast.from.24.01.2025.15.58.47.webm
I also noticed that's possible to change cursor position of the underlying text area using arrows.
The code lgtm regarding the structure, but I cannot understand and follow in sb the LoginWithKeycardFlow path with locked keycard.
| @@ -15,6 +15,7 @@ Dialog { | |||
|
|
|||
| required property string password | |||
| required property string pin | |||
| required property string selectedProfileIsKeycard | |||
There was a problem hiding this comment.
looks like not property of that component, can be defined in the place where the component is used probably
| @@ -192,22 +203,47 @@ KeycardBasePage { | |||
| } | |||
| }, | |||
| State { | |||
| name: "locked" | |||
There was a problem hiding this comment.
I wondering about decomposing that component into smaller per-state chunks. Having multiple states with multiple property changes generates a lot of changes when adding sth there.
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
Lockedkeycard state everywhere in favor ofBlockedPINandBlockedPUKFixes: #17092
Fixes: #17109
Iterates: #17098
Affected areas
Onboarding
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
Unblocking with PUK from both Onboarding and Login screens:
Zaznam.obrazovky.z.2025-01-23.15-16-58.mp4