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

[ProfileSettings]: Added biometrics in password change view #13669

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

alexandraB99
Copy link
Contributor

@alexandraB99 alexandraB99 commented Feb 21, 2024

Closes #13302

What does the PR do

ProfileSettings: Added biometrics in password change view

Affected areas

ProfileSettings password change view

Screenshot of functionality (including design for comparison)

ff.mov

@status-im-auto
Copy link
Member

status-im-auto commented Feb 21, 2024

Jenkins Builds

Click to see older builds (24)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ ead13a2 #1 2024-02-21 17:10:09 ~6 min tests/nim 📄log
✔️ ead13a2 #1 2024-02-21 17:11:49 ~8 min macos/aarch64 🍎dmg
✔️ ead13a2 #1 2024-02-21 17:13:44 ~9 min macos/x86_64 🍎dmg
✔️ ead13a2 #1 2024-02-21 17:14:26 ~10 min tests/ui 📄log
✔️ ead13a2 #1 2024-02-21 17:20:50 ~17 min linux/x86_64 📦tgz
✔️ ead13a2 #1 2024-02-21 17:37:50 ~34 min windows/x86_64 💿exe
✔️ 314f26f #2 2024-02-22 10:13:33 ~8 min macos/aarch64 🍎dmg
✔️ 314f26f #2 2024-02-22 10:13:44 ~8 min macos/x86_64 🍎dmg
✔️ 314f26f #2 2024-02-22 10:15:41 ~10 min tests/nim 📄log
✔️ 314f26f #2 2024-02-22 10:22:08 ~17 min tests/ui 📄log
✔️ 314f26f #2 2024-02-22 10:22:27 ~17 min linux/x86_64 📦tgz
✔️ 314f26f #2 2024-02-22 10:33:48 ~28 min windows/x86_64 💿exe
✔️ 4d6977b #3 2024-02-23 10:18:53 ~5 min macos/aarch64 🍎dmg
✔️ 4d6977b #3 2024-02-23 10:21:01 ~7 min tests/nim 📄log
✔️ 4d6977b #3 2024-02-23 10:22:01 ~8 min macos/x86_64 🍎dmg
✔️ 4d6977b #3 2024-02-23 10:26:10 ~12 min tests/ui 📄log
✔️ 4d6977b #3 2024-02-23 10:31:36 ~17 min linux/x86_64 📦tgz
✔️ 4d6977b #3 2024-02-23 10:45:24 ~31 min windows/x86_64 💿exe
✔️ 736918b #4 2024-02-27 17:38:16 ~5 min tests/nim 📄log
✔️ 736918b #4 2024-02-27 17:38:24 ~5 min macos/aarch64 🍎dmg
✔️ 736918b #4 2024-02-27 17:42:49 ~10 min tests/ui 📄log
✔️ 736918b #4 2024-02-27 17:47:29 ~15 min linux/x86_64 📦tgz
✔️ 736918b #4 2024-02-27 17:56:50 ~24 min windows/x86_64 💿exe
✔️ 736918b #4 2024-02-27 18:41:50 ~1 hr 9 min macos/x86_64 🍎dmg
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 1f0a796 #5 2024-02-28 09:11:27 ~4 min macos/aarch64 🍎dmg
✔️ 1f0a796 #5 2024-02-28 09:13:04 ~6 min tests/nim 📄log
✔️ 1f0a796 #5 2024-02-28 09:17:10 ~10 min tests/ui 📄log
✔️ 1f0a796 #5 2024-02-28 09:17:28 ~10 min macos/x86_64 🍎dmg
✔️ 1f0a796 #5 2024-02-28 09:23:16 ~16 min linux/x86_64 📦tgz
✔️ 1f0a796 #5 2024-02-28 09:37:35 ~30 min windows/x86_64 💿exe
✔️ b0a3909 #6 2024-02-28 16:44:04 ~6 min tests/nim 📄log
✔️ b0a3909 #6 2024-02-28 16:44:21 ~6 min macos/aarch64 🍎dmg
✔️ b0a3909 #6 2024-02-28 16:48:31 ~10 min macos/x86_64 🍎dmg
✔️ b0a3909 #6 2024-02-28 16:49:16 ~11 min tests/ui 📄log
✔️ b0a3909 #6 2024-02-28 16:53:08 ~15 min linux/x86_64 📦tgz
✔️ b0a3909 #6 2024-02-28 17:06:56 ~29 min windows/x86_64 💿exe

Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

Also looks wrong when the other popup is asking for password, the Switch is unchecked by itself?

Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

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

I have a few suggestions below.

Another thing I found is that the buttons are cut when the popup is opened the first time.

Screenshot 2024-02-22 at 10 15 14


readonly property bool biometricsEnabled: localAccountSettings.storeToKeychainValue === Constants.keychain.storedValue.store
onBiometricsEnabledChanged: {
biometricsSwitch.checked = Qt.binding(() => { return root.biometricsEnabled });
Copy link
Contributor

Choose a reason for hiding this comment

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

The biometricsSwitch is in a loader. On the first event the biometricsSwitch may not exist. An undefined check would be useful here.

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 will change only when the switch is checked/unchecked and the biometrics are enabled/disabled. so it should be good?

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a warning when this component gets loaded that the biometricsSwitch is undefined. We could remove it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexjba I don't see any warning but I've added the check anyways. in regards to the buttons cut, I can't see that one either

}
StatusModal {
id: enableBiometricsPopup
title: biometricsSwitch.checked ? qsTr("Disable biometrics") : qsTr("Enable biometrics")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd link it to the root.biometricsEnabled as it's supposed to be the single source of truth for this feature. Looks like the biometricsSwitch.checked can have intermediary states. Just to make sure we don't have false positive/negative here.

Same below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, however at this stage, we have not yet enabled or disabled biometrics, so localAccountSettings.storeToKeychainValue hasn't changed yet

StatusButton {
text: biometricsSwitch.checked ? qsTr("Yes, enable biometrics") : qsTr("Yes, disable biometrics")
onClicked: {
if (biometricsSwitch.checked && !biometricsSwitch.biometricsEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (biometricsSwitch.checked && !biometricsSwitch.biometricsEnabled) {
if (biometricsSwitch.checked && !root.biometricsEnabled) {

@alexandraB99
Copy link
Contributor Author

I have a few suggestions below.

Another thing I found is that the buttons are cut when the popup is opened the first time.
Screenshot 2024-02-22 at 10 15 14

I cannot reproduce this one, any specific steps?

@alexjba
Copy link
Contributor

alexjba commented Feb 22, 2024

I have a few suggestions below.
Another thing I found is that the buttons are cut when the popup is opened the first time.
Screenshot 2024-02-22 at 10 15 14

I cannot reproduce this one, any specific steps?

Try restarting the app and opening this popup once. It's happening only on the first open

@alexandraB99
Copy link
Contributor Author

Also looks wrong when the other popup is asking for password, the Switch is unchecked by itself?

not sure how to react on the password's popup closing signal.

@alexandraB99
Copy link
Contributor Author

I have a few suggestions below.
Another thing I found is that the buttons are cut when the popup is opened the first time.
Screenshot 2024-02-22 at 10 15 14

I cannot reproduce this one, any specific steps?

Try restarting the app and opening this popup once. It's happening only on the first open

still can't see it

@alexandraB99
Copy link
Contributor Author

Bug for popup buttons cut is created and will be fixed here: #13760

cc @alexjba

Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks :D

Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@alexandraB99 alexandraB99 merged commit cdbe3a7 into master Feb 29, 2024
8 checks passed
@alexandraB99 alexandraB99 deleted the feat/issue-13302 branch February 29, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants