Remove traces of identity rings in the mobile app #22360
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilmotta
requested review from
Parveshdhull,
flexsurfer,
seanstrom and
ulisesmac
Parveshdhull
approved these changes
Mar 25, 2025
Member
Jenkins BuildsClick to see older builds (4)
|
flexsurfer
approved these changes
Mar 25, 2025
Horupa-Olena
force-pushed
the
ilmotta-21743/remove-identicon-ring
branch
from
37b98c0 to
2ec7f9e
Compare
|
@ilmotta Thank you for your PR! 1. High contrast of default avatar colors in iOS dark mode
Suggestion: Since these colors are not used often (most people upload photos), we could simply change the order so blue is not the first and doesn’t get set automatically. 2. Avatars blending with the background Suggestion: Add a thin border around avatars. The tricky part is that the border should be light in dark mode and dark in light mode. Let me know what you think! |
|
@ilmotta I have reviewed the following areas where avatars are displayed for regular user, for user witn ENS and for Keycard flow.
Also was rechek areas where ring don't should displayed fpr missing any regression/ |
ulisesmac
approved these changes
Mar 25, 2025
Contributor
ulisesmac
left a comment
ulisesmac
left a comment
There was a problem hiding this comment.
Thanks for the PR @ilmotta
I'll miss the ring, I know itself isn't an efficient validation mechanism, but along with the emojihash and compressed key, it could've worked; however I understand we want something simpler for our users.
I'm willing to see new proposals to make this better 👍
Contributor
|
@Horupa-Olena Very interesting comments, I'm glad to see QA is also paying attention to the usability and comfort while using the app, let's see what's the output |
Member
43% of end-end tests have passedFailed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestWalletCollectibles:
Passed tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestWalletCollectibles:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Member
91% of end-end tests have passedFailed tests (2)Click to expandClass TestFallbackMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (4)Click to expandClass TestCommunityMultipleDeviceMergedThree:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletCollectibles:
Passed tests (62)Click to expandClass TestDeepLinksOneDevice:
Class TestAndroid12:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestAndroid13:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestFallbackMultipleDevice:
Class TestWalletMultipleDevice:
Class TestWalletCollectibles:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedThree:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
|
|
PR can be merged, but still wait for the @ilmotta opinion on these points. |
Contributor
Author
|
Hi @Horupa-Olena, excellent QA assessment in this PR as @ulisesmac reinforced as well 💯 Originally, a few months ago I opened a discussion with the Design team and one of the points I asked was I'll proceed to merge the PR since you approved, but feel free to reopen the discussion in Discord pointing to my original discussion even. Perhaps with your example screenshots the ideal solution will be reconsidered and we could implement that in the future. |
ilmotta
force-pushed
the
ilmotta-21743/remove-identicon-ring
branch
from
2ec7f9e to
653b223
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #21743
Summary
This PR removes the identity ring (colorful ring around avatars) from the mobile client. status-go isn't going to be changed until status-desktop is updated as well.
Why: there's general consensus that the ring colors are a flawed mechanism against impersonation. Further details in issue #21743 and original discussion in #20617. There's agreement from the Design team that the feature should be removed and Figma is either up-to-date without rings or is in the process of being updated.
We are not attempting to resolve the impersonation problem right now. Eventually, we will return to the drawing board and come up with better solutions.
Areas that may be impacted
Probably the impact, if it happens, would be solely on the UI. I checked some instances of avatars in the app and they look correct without the ring.
Steps to test
Verify all possible places where avatars are displayed. For example, profile (settings) screen, editing profile image, lists of group members, lists of community/channel members, login profile list, sending a contact request, replying to a message, etc. Consider the case where there's an ENS name set as well because we weren't already showing rings when ENS names were set.
status: ready