-
Notifications
You must be signed in to change notification settings - Fork 86
feat(@desktop/wallet): Adding UI flows for Enabling/Disabling Third #18684
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
base: master
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (26)
|
fd95753
to
f35d360
Compare
83750de
to
255167b
Compare
party services fixes #18510
255167b
to
6171b2b
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.
Looks good in general! Just some small objections
// Verify visibility | ||
tryCompare(thirdPartyServices, "visible", true) | ||
|
||
waitForRendering(page) |
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.
Why wait here and then not check anything? 😄
@@ -64,6 +67,11 @@ OnboardingStackView { | |||
|
|||
signal finished(int flow) | |||
|
|||
// Thirdparty services | |||
property bool privacyModeFeatureEnabled | |||
property bool thirdpartyServicesEnabled |
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.
make them required
please
|
||
SettingsContentBase { | ||
id: root | ||
|
||
property bool isStatusNewsViaRSSEnabled | ||
required property bool isCentralizedMetricsEnabled | ||
property bool thirdpartyServicesEnabled | ||
property bool privacyModeFeatureEnabled |
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.
required
|
||
import utils | ||
|
||
StatusModal { |
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.
StatusDialog
in new code pls
anchors.right: parent.right | ||
anchors.rightMargin: Theme.padding | ||
anchors.leftMargin: Theme.padding | ||
anchors.topMargin: Theme.padding |
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.
Yay anchors
🫢
rightButtons: [ | ||
StatusButton { | ||
type: StatusBaseButton.Type.Primary | ||
normalColor: Theme.palette.customisationColors.purple |
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.
As elsewhere, purple on purpose?
@@ -1012,6 +1012,8 @@ QtObject { | |||
readonly property string statusHelpLinkPrefix: `https://status.app/help/` | |||
readonly property string downloadLink: "https://status.im/get" | |||
readonly property string sendViaChatPrefix: '//send-via-personal-chat//' | |||
readonly property string statusDiscussPageUrl: 'https://discuss.status.app/c/features/51' |
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.
statusDiscussPageUrl
sounds like a generic page, eventhough it points to a very specific one (Feature requests)
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.
Here some comments added:
Issues found while testing:
- The toggle doesn't work properly since we need a "doble-confirmation". I think the toggle should be reevaluated after closing the popup so that it really takes the real state. Here the video:
Screen.Recording.2025-09-05.at.13.22.25.mov
Design questions:
- The only way to enable or disable privacy mode during the onboarding process is with a new fresh account. If I'm already logged in with an account, even if I create a new one, the page / flow won't be accessible. Perhaps we should consider other onboarding flows as well. WDYT @Khushboo-dev-cpp @xAlisher?
thirdpartyServicesEnabled: ctrlThirdpartyServicesEnabled.checked | ||
privacyModeFeatureEnabled: ctrlPrivacyMode.checked | ||
onOpenThirdpartyServicesInfoPopupRequested: { | ||
console.warn("onLaunchThirdyPartyServicesInfoPopupRequested") |
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.
Suggestion: You could use the log mechanism we normally use inside storybook
so that the log is already visible inside the same environment!
visible: true | ||
closePolicy: Popup.CloseOnEscape | ||
thirdPartyServicesEnabled: ctrlThirdpartyServicesEnabled.checked | ||
onToggleThirdpartyServicesEnabled: console.warn("onToggleThirdpartyServicesEnabled called ") |
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.
Same here!
@@ -64,6 +67,11 @@ OnboardingStackView { | |||
|
|||
signal finished(int flow) | |||
|
|||
// Thirdparty services | |||
property bool privacyModeFeatureEnabled |
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.
Is this a wip one, right? Maybe a comment to understand that this will be removed once we remove the feature flag would help!
// Thirdparty services | ||
property bool privacyModeFeatureEnabled | ||
property bool thirdpartyServicesEnabled | ||
signal toggleThirdpartyServicesEnabled() |
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.
suggestion: We are trying to unify nomenclarture
signal toggleThirdpartyServicesEnabled() | |
signal toggleThirdpartyServicesEnabledRequested() |
@@ -15,13 +15,15 @@ import utils | |||
OnboardingPage { | |||
id: root | |||
|
|||
title: qsTr("Welcome to Status") | |||
property bool privacyModeFeatureEnabled | |||
property bool thirdpartyServicesEnabled |
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.
property bool thirdpartyServicesEnabled | |
required property bool thirdpartyServicesEnabled |
@@ -44,4 +44,12 @@ QtObject { | |||
function mnemonicWasShown() { | |||
root.privacyModule.mnemonicWasShown() | |||
} | |||
|
|||
readonly property bool thirdpartyServicesEnabled: appSettings.thirdpartyServicesEnabled |
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.
It would be nice if we access to the appSettings
context property via an instance 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.
Seeing now this property in here, I think that there is a bit of messed up nomenclature with this other property from the comment here? #18750 (comment)
subTitle: qsTr("Enable/disable all third-party services") | ||
components: [ | ||
StatusSwitch { | ||
checked: root.thirdpartyServicesEnabled |
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.
Already written in the global comments.. There's a double-check
action needed, so that, there's a middle state a bit confusing
party services
fixes #18510
status-go PR : status-im/status-go#6865
What does the PR do
Summary
This PR introduces the Third-Party Services feature with full UI integration, feature-flag control, and persistence in the settings database.
Key Changes
Covered Flows
Notes
The feature is disabled by default and controlled via the feature flag.
When enabled, users can opt-in/out through the UI switch, and the setting is saved in the DB on go side
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
Screen.Recording.2025-08-28.at.13.59.09.mov
Impact on end user
How to test
Risk