feat(Onboarding) Implement new Login screen #17057
Merged
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
force-pushed
the
feat/onboarding-new-login-screen
branch
2 times, most recently
from
544dd34 to
f6b898d
Compare
caybro
added a commit
that referenced
this pull request
Jan 10, 2025
Jenkins BuildsClick to see older builds (178)
|
caybro
added a commit
that referenced
this pull request
Jan 10, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
f6b898d to
7be9676
Compare
caybro
added a commit
that referenced
this pull request
Jan 10, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
7be9676 to
401e2e0
Compare
caybro
added a commit
that referenced
this pull request
Jan 11, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
401e2e0 to
b688aae
Compare
caybro
added a commit
that referenced
this pull request
Jan 12, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
b688aae to
c263dfc
Compare
micieslak
force-pushed
the
feat/onboarding-rework-1
branch
2 times, most recently
from
d5524fd to
35b8620
Compare
caybro
added a commit
that referenced
this pull request
Jan 13, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
c263dfc to
73ddd3b
Compare
anastasiyaig
force-pushed
the
feat/onboarding-rework-1
branch
from
29ce4b0 to
43a5b68
Compare
caybro
added a commit
that referenced
this pull request
Jan 13, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
73ddd3b to
0557b49
Compare
caybro
added a commit
that referenced
this pull request
Jan 13, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
0557b49 to
a9fecc4
Compare
caybro
added a commit
that referenced
this pull request
Jan 14, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
a9fecc4 to
5e00603
Compare
caybro
added a commit
that referenced
this pull request
Jan 14, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
5e00603 to
589e17c
Compare
caybro
added a commit
that referenced
this pull request
Jan 14, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
589e17c to
0157c62
Compare
caybro
added a commit
that referenced
this pull request
Jan 15, 2025
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
0157c62 to
e788759
Compare
caybro
changed the title
caybro
added a commit
that referenced
this pull request
Jan 21, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
c7d2222 to
8712ca6
Compare
micieslak
reviewed
Jan 21, 2025
ui/app/AppLayouts/Onboarding2/controls/LoginUserSelectorDelegate.qml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
As the LoginPage name is already free, can we rename this one to LoginPage?
There was a problem hiding this comment.
🤷 dunno, I liked the LoginScreen better but yeah, I can rename it
There was a problem hiding this comment.
Two more reasons, I never liked the LoginPagePage pattern for storybook, and also this is not really a "page" in the sense of the onboarding; rather a separate thing
There was a problem hiding this comment.
now we have flows, pages and layout, combining all the things together. Screen just doesn't fit to that convention for me, structurally it's like other "pages" we have. "PagePage" is Storybook's problem and shouldn't affect naming patterns in the app.
Comment on lines
49
to
+59
| function init() { | ||
| root.stackView.push(welcomePage) | ||
| } | ||
|
|
||
| function startCreateProfileFlow() { | ||
| root.stackView.push(createProfilePage) | ||
| } | ||
|
|
||
| function startLoginFlow() { | ||
| root.stackView.push(loginPage) | ||
| } |
There was a problem hiding this comment.
Actually all three method do initialization of the flow, the difference is starting page. So it would be nice to align naming somehow.
What about changing init into startWelcomeFlow or startWelcomePage?
Otherwise it's easy to think that it's necessary to call init first, even before calling e.g. startCreateProfileFlow.
There was a problem hiding this comment.
Yeah I'll think about something, as I'm adding more things in the followup PR
storybook/pages/LoginScreenPage.qml
Outdated
| onSelectedProfileKeyIdChanged: biometricsPopup.visible = Qt.binding(() => ctrlBiometrics.checked && ctrlTouchIdUser.checked && store.keycardState === Onboarding.KeycardState.NotEmpty) | ||
| } | ||
|
|
||
| Dialog { |
There was a problem hiding this comment.
It could be excluded to a separate file, could be stored in pages dir as some other utilities that are intended to be used only from pages and are not general purpose storybook utils.
There was a problem hiding this comment.
Yeah good idea; I was thinking about the same for the "keycard state" buttons/combobox, it's just duplicated all over the place
There was a problem hiding this comment.
Yup, extracted to a separate component
There was a problem hiding this comment.
putting it into status-desktop/storybook/src/Storybook/BiometricsPopup.qml
breaks the convention. storybook/src/Storybook/ is location of generic storybook components that could be used in any project. App-specific helpers should go directly to pages dir.
caybro
added a commit
that referenced
this pull request
Jan 22, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
8712ca6 to
435e210
Compare
caybro
added a commit
that referenced
this pull request
Jan 22, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
435e210 to
0cdbe5b
Compare
|
@micieslak I think I handled and fixed all the comments, pls have a second look; same for @jrainville. Would be nice to be able to merge this so that we have at least some integration to work with |
caybro
added a commit
that referenced
this pull request
Jan 22, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
micieslak
reviewed
Jan 23, 2025
Comment on lines
27
to
30
| let uid = keyUid | ||
| if (uid === "") | ||
| uid = proxyModel.get(0, "keyUid") // select the first entry from the model | ||
| currentEntry.value = uid |
There was a problem hiding this comment.
Suggested change
| let uid = keyUid | |
| if (uid === "") | |
| uid = proxyModel.get(0, "keyUid") // select the first entry from the model | |
| currentEntry.value = uid | |
| currentEntry.value = keyUid === "" ? (proxyModel.get(0, "keyUid") ?? "") : keyUid |
There are several potential problems in the imperative code fetching the first entry key:
- Model may be empty and the fetched value is undefied (fixed by the proposed code)
- If model is empty when
setSelectionis called and populated later, first entry won't be fetched - If the provided
keyUidis not in the model, fallback to the first entry won't work
That situation shows limitations of ModelEntry which is limited it terms of how the entry is pointed. The solution could be generalizing the ModelEntry to support more generic search criteria.
I created ticket proposing changes to ModelEntry which could allow handling cases like that in simple, declarative way: #17117
ui/app/AppLayouts/Onboarding2/controls/LoginUserSelectorDelegate.qml
Outdated
Show resolved
Hide resolved
- explicitely provide a bg color - also fix the mouse cursor shape
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
caybro
force-pushed
the
feat/onboarding-new-login-screen
branch
from
0cdbe5b to
40e8411
Compare
caybro
added a commit
that referenced
this pull request
Jan 23, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
caybro
added a commit
that referenced
this pull request
Jan 23, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
micieslak
approved these changes
Jan 23, 2025
I did move it to a shared location: Or did you mean something else? |
caybro
added a commit
that referenced
this pull request
Jan 23, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
caybro
added a commit
that referenced
this pull request
Jan 23, 2025
- implement the new UI and frontend logic of the Login screen - integrate it (as a separate page) into the OnboardingLayout - add SB pages - add an integration QML test - add some TODOs and FIXMEs for the existing and new external flows, which will be covered separately in followup PRs Fixes #17057
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
TODO for followup PRs:
UnblockWithPukFlow#17111Fixes #17049
Fixes #17081
Fixes #17100
Affected areas
Login screen
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
OnboardingLayout SB page (showing the Login screen):
Selecting a different user/profile:
Password:
Keycard/biometrics:
