-
Notifications
You must be signed in to change notification settings - Fork 86
feat(@desktop/privacy): Added privacy wall screens for when third party #18750
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: feat/toggleThirdPartyServices
Are you sure you want to change the base?
feat(@desktop/privacy): Added privacy wall screens for when third party #18750
Conversation
Jenkins Builds
|
…Mode for the navigation bar fixes #18511
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 in general, jsut some small things
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.
Those PNG screenshots are unusually big (as in filesize) for their small size... have you tried running optipng
on them?
@@ -856,6 +856,9 @@ Item { | |||
} | |||
return username | |||
} | |||
|
|||
readonly property bool thirdPartyServicesDisbaled: featureFlagsStore.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.
readonly property bool thirdPartyServicesDisbaled: featureFlagsStore.privacyModeFeatureEnabled && | |
readonly property bool thirdPartyServicesDisabled: featureFlagsStore.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.
Just a typo
dappMetrics.logNavigationEvent(DAppsMetrics.DAppsNavigationAction.DAppConnectInitiated) | ||
dAppsServiceLoader.dappConnectRequested() | ||
} | ||
onDappDisconnectRequested: (dappUrl) => { |
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.
See how the indentation is messed up inside the lambda... it's a QtCreator bug indeed but that's why I actually prefer the function(dappUrl) {}
syntax 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.
Great job @Khushboo-dev-cpp !
Tested on desktop and mobile. I've posted some comments in code and here are some other issues and suggestions I want to share:
Issues:
- In wallet section,
Organize collectibles your way
- 3rd carousel image - the title is missing:

- Wrong word here when in market section:

- We should remove this selection
Open in Status
for mobile platforms since browser is not available there yet and if selected, it drives the app to a white screen non return page:
Screen.Recording.2025-09-05.at.12.47.12.mov
Design suggestions / concerns:
- I think we should change the selected section icon color in light mode when in privacy mode since it's vaguely visible (also the hover color):

- I think it would be nicer if the screenshots on dark mode are also shown on dark mode.
- Wdyt if we add some smooth transition in between each screenshot. I feel they appear abruptly. OR, maybe extending a bit the time for each image?
cc: @xAlisher for the design questions
|
||
MarketPrivacyWall { | ||
onOpenThirdpartyServicesInfoPopupRequested: console.warn("Enable third party services requested") | ||
onOpenDiscussPageRequested: console.warn("Open discuss page requested") |
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 create a Logs
object and use the logs.logEvent
so that we can see it inside a LogsAndControlsPanel
component directly on the storybook
environment
} | ||
} | ||
|
||
onEnableThirdpartyServicesRequested: console.warn("Enable third party services requested") |
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 comment than before!
id: root | ||
|
||
WalletPrivacyWall { | ||
onOpenThirdpartyServicesInfoPopupRequested: console.warn("Enable third party services requested") |
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 comment than before! ;)
@@ -27,7 +29,8 @@ Rectangle { | |||
|
|||
implicitWidth: 78 | |||
|
|||
color: Theme.palette.statusAppNavBar.backgroundColor | |||
color: root.thirdpartyServicesDisabled ? | |||
Theme.palette.customisationColors.purple : Theme.palette.statusAppNavBar.backgroundColor |
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.
Wondering if it would make sense to create a specific property in Theme.palette
called something like privacyModeColor
and do the asignement of the specific color inside the palette component?
image: "wallet/placeholders/swapView" | ||
} | ||
ListElement { | ||
header: qsTr("Organize collectibles your 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.
I guess this is the issue I posted on the global comment:
header: qsTr("Organize collectibles your way") | |
primary: qsTr("Organize collectibles your way") |
@@ -856,6 +856,9 @@ Item { | |||
} | |||
return username | |||
} | |||
|
|||
readonly property bool thirdPartyServicesDisbaled: featureFlagsStore.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.
Would it make sense to create a property inside the main root store (ui/app/AppLayouts/stores/RootStore.qml) instead since it seems a really global setting for the app? I see it at the same / similar level than isProduction
or isOnline
properties.
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.
Having it on shared folder
makes me feel it's a statusq
one ¿maybe?
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 a potential component to test on storybook
?
font.pixelSize: Theme.additionalTextSize | ||
wrapMode: Text.WordWrap | ||
|
||
text: qsTr("Enable third-party services for wallet features to work.") |
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 should be customizable as well, so, or you add a new param in the model, or a property to set it up from outside.
Here it's the other issue described on the global comments, in case of market tab
, the string changes slightly.
What does the PR do
Implements the Privacy walls for Wallet and Market tabs when Thirdparty services are disabled as defined in designs here https://www.figma.com/design/idUoxN7OIW2Jpp3PMJ1Rl8/Settings----Desktop-Legacy?node-id=26301-22159&m=dev
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
Screen.Recording.2025-08-29.at.15.07.58.mov
Screen.Recording.2025-08-29.at.15.44.43.mov
Impact on end user
How to test
Risk