-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
Jenkins BuildsClick to see older builds (24)
|
There was a problem hiding this 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
readonly property bool biometricsEnabled: localAccountSettings.storeToKeychainValue === Constants.keychain.storedValue.store | ||
onBiometricsEnabledChanged: { | ||
biometricsSwitch.checked = Qt.binding(() => { return root.biometricsEnabled }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (biometricsSwitch.checked && !biometricsSwitch.biometricsEnabled) { | |
if (biometricsSwitch.checked && !root.biometricsEnabled) { |
ead13a2
to
314f26f
Compare
not sure how to react on the password's popup closing signal. |
736918b
to
1f0a796
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks :D
1f0a796
to
b0a3909
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
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