-
Notifications
You must be signed in to change notification settings - Fork 984
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
[#11468] Hide/show option for accounts in the wallet #12438
Conversation
Jenkins BuildsClick to see older builds (15)
|
1% of end-end tests have passed
Failed tests (70)Click to expand
Passed tests (1) |
1% of end-end tests have passed
Failed tests (70)Click to expand
Passed tests (1) |
a5c0721
to
1702fdc
Compare
96% of end-end tests have passed
Failed tests (3)Click to expand
Passed tests (68)Click to expand |
94% of end-end tests have passed
Failed tests (4)Click to expand
Passed tests (67)Click to expand |
@flexsurfer thanks for PR! ISSUE 1: No
|
@flexsurfer apologies for the late review! Is it an option for you to pause this work without it going stale in the next days or week? As wallet lead I think it'll be good if @carlfairclough has a chance to review |
@hesterbruikman sure , np |
thanks @qoqobolo , fixed |
Thanks for highlighting this @hesterbruikman. @flexsurfer just FYI we have some upcoming settings changes that may alter the architecture of this as we need to incorporate:
That being said, I think we should go ahead and merge functionality that is ready now, rather than wait. I would suggest that we alter the list items to have a shown/hidden icon rather than a checkbox. It makes it a little more clear what the feature does. If you need additional UI guidance please let me know Icons: |
thanks @carlfairclough |
b358499
to
735219a
Compare
735219a
to
4f75f0d
Compare
@qoqobolo ready for retesting |
@flexsurfer thank you! Tested on Android 11 and iOS 14:
No issues found. Could you add accessibility-ids to these icons, please? |
4f75f0d
to
47cc27a
Compare
@qoqobolo by default they have label "icon" do you need a different one? |
@flexsurfer |
47cc27a
to
223f269
Compare
ok , done |
90% of end-end tests have passed
Failed tests (7)Click to expand
Passed tests (64)Click to expand |
@qoqobolo could you please confirm that we can merge and failed e2e not related, thanks |
e2e failures are not related to PR. |
223f269
to
ad9dc99
Compare
mhm why has it closed status but not merged |
fixes #11468