Anthony toggle show password

[anthony-hull]

Details

Added a toggle button icon to password fields to change visibility

Fixed Issues

#2734

QA Steps

Web and Desktop:

1. Type something into a password field
2. Verify password is hidden
3. Click eye icon
4. Verify the password is visible
5. Click again
6. Verify it is hidden again

Android and iOS

1. Type something into a password field
2. Verify password is shown
3. Click eye icon
4. Verify the password is hidden
5. Click again
6. Verify it is shown again

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

setpassword screen web:
https://user-images.githubusercontent.com/47184118/129957727-cff28fb8-f9f5-4d6c-9d69-cd469e2e74e7.mp4

Edge

note the lack of vendor specific show password functionality

Mobile Web

Desktop

Screen.Recording.2021-09-15.at.18.57.37.mov

iOS

Android

[anthony-hull]

I have yet to upload all the proof of testing as I have an outstanding implementation question in the PR

[johnmlee101]

Where is the question?

[anthony-hull]

it is inside the review I started. you need to click view changes:

[johnmlee101]

@anthony-hull your review is still Pending you need to close it out by clicking Comment for us to see it.

[anthony-hull]

@johnmlee101 can you see my review now?

[johnmlee101]

Yes!

[anthony-hull]

I just have to fix what broke in the merge

[parasharrajat]

I noticed that in the Edge browser Eye icon is already shown to reveal the password. Are you going to disable that? Otherwise, two icons will be shown.

IMO, You should create a wrapper component around ExpensiInput for password input. This eye is only going to be used for Password inputs thus this code is useless 90% of the time for ExpensiInput usages.

[parasharrajat]

I was thinking if we increase the space around the icon horizontally then it will look better. But I tagged you to ask about this #4746 (comment) @shawnborton

[shawnborton]

Ah, got it. I like the mockups where there is more space between the 1Pass icon and the eye. Maybe we can just use 8px gap between those two icons and see how it looks?

[anthony-hull]

top is with 8px
second is both overlaid

[shawnborton]

Let's go with the top version.

[anthony-hull]

Just need to test on all platforms but something has come up this evening. I'll be able to carry on tomorrow

[parasharrajat]

Is this ready for review?

[anthony-hull]

It works on Web, Mobile Web and Desktop. I can't test on the native apps at the moment as they crash due to another issue.
It's otherwise ready for review

[parasharrajat]

The crash is fixed now. You can pull new changes and test.

[anthony-hull]

oh man tired of fixing breaking changes from merges 😆

[anthony-hull]

@parasharrajat testing complete, please review

[parasharrajat]

I hate to say this but overflow issue is back

[parasharrajat]

Just an overflow issue again and a few tweaks. I think that the current implementation could be improved by using an absolute positioned icon over the input. But as this is working fine, I won't ask you to do that unless you are willing to do that change.

[anthony-hull]

Just an overflow issue again and a few tweaks. I think that the current implementation could be improved by using an absolute positioned icon over the input. But as this is working fine, I won't ask you to do that unless you are willing to do that change.

If it were absolute, wouldn't any password manager icons render underneath it? as the input element would fill the space due to the icon being out of flow?

[parasharrajat]

If it were absolute, wouldn't any password manager icons render underneath it? as the input element would fill the space due to the icon being out of flow?

Good point. Then we are good.

[parasharrajat]

Hey, @anthony-hull I forgot to mention earlier that you have to sign all of your commits before we can merge the PR.

[parasharrajat]

@anthony-hull Any update on the requested changes. Let's complete this asap. Let me know if you are facing any challenges.

[anthony-hull]

I've fixed the overflowing input. I'll sign the commits, re-implement the negative margin and then re-test. should be ready soon!

[anthony-hull]

do I have to have all of them to show as verified? I ran a rebase command to sign them all and I was having fix merge issues on all my changes again. git was not happy. it was a nightmare so I aborted.

git rebase --exec 'git commit --amend --no-edit -n -S' -i main

[parasharrajat]

Unfortunately Yes.

[anthony-hull]

much easier to make a new PR with a new branch :)
please see:
#6501
I just need to test on all platforms now. I'll let you know!

[parasharrajat]

@anthony-hull Please close this PR. Thanks.