-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: add privacy toggle #8234
feat: add privacy toggle #8234
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
756c61c
to
3f70a36
Compare
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/70d1699d-6d0e-40e5-aeba-735ebfcec33e |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/58f823e6-cd4e-454e-ab76-5eb8bf3246f8 |
@salimtb There was a ticket 2 of 3 prior to this iteration. Is it correct to say those changes are also implemented in this PR? When you have the chance, could you also verify that the network verification errors are still being respected when adding network from both custom and dapps? A video would be fine |
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.
Awesome work!
Just some comments:
-
It seems that comparing it with figma is missing bold on the copy
Settings -> Security and Privacy
part
-
Can we also have a scenario with the recordings of the scenario with warning alerts?
-
- When added a chain via Dapp
-
- When adding a chain via Custom Network
-
It seems that I started the first PR of this Batch and I missed it or it was added to the Figma after, but the banners are missing a cross icon to be able to closed on the right side :( , let me know if you can add it!
Figma:
Recording from the first batch PR
- Also it makes sense to add a condition before we check the networks here , we do not want to fetch anything if the toggle is disabled
NIT:
Do you think it would be better extracting the contextual sheet of enabling/disabling the new privacy toggle would be better for easier change/reusability on the future?
Also wanted to bring up a product question, I can see that on slide 28 we do show intention to show the warning banners of the network when adding it from our popular list. It seems just new noise to the user, let me know your thoughts |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@tommasini Let me see if I understand the logic you're proposing correctly:
If so I agree. And we can probably makes this smarter as a next step as well. This would be the sequence of implementation and logic:
@alfeng6 Wdyt? I do have reservation about my own proposal as a last step. UX improvement, but we should generally avoid making strong recommendations |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
a140545
to
897543f
Compare
5af7e3c
to
a748747
Compare
Hi @salimtb Recording showing approve and switch views not shown on android: https://recordit.co/82bDXlPoXa |
c5826a1
to
d04931b
Compare
d04931b
to
49943d2
Compare
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 7 New issues |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8de3fb2b-dcc1-4750-88e8-a2e2a60321f0 |
Description
This task aims to add the ability to turn on and off the network verification feature. To do that, we will be adding both a toggleable section in the settings as well as a contextual banner and sheet for quickly toggling it on.
Related issues
Fixes: #1269
Manual testing steps
Screenshots/Recordings
Before
After
https://drive.google.com/drive/folders/1BeTFKHelGrzu5XPUfXLTa-om7inhtowU?usp=drive_link
Pre-merge author checklist
Pre-merge reviewer checklist