add show password toggle to secureTextInput #6501

anthony-hull · 2021-11-29T16:56:31Z

New PR continued from here due to GPG signing issues:
#4746

Details

Added a toggle button icon to password fields to change visibility

Fixed Issues

$ #2734

QA Steps

Web and Desktop:

Type something into a password field
Verify password is hidden
Click eye icon
Verify the password is visible
Click again
Verify it is hidden again

Android and iOS

Type something into a password field
Verify password is shown
Click eye icon
Verify the password is hidden
Click again
Verify it is shown again

Tested On

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

parasharrajat · 2021-11-29T17:13:44Z

@anthony-hull please add $ before the GH issue link. It's important for automation.

$ https://github.com/Expensify/App/issues/2734

parasharrajat · 2021-11-29T17:31:10Z

cc: @johnmlee101 for review.

anthony-hull · 2021-11-29T17:31:26Z

@johnmlee101 @parasharrajat
here is the new PR. I re-created a new branch with my changes as a single signed commit to fix the GPG signature issues on the other branch

@parasharrajat
Can you please review this again? :)

johnmlee101

I'll let @parasharrajat do final review 😄

parasharrajat

There is only a bug remaining and a few tweaks needed for the code.

I won't bother you much so I have shared the fix in the review comments.

src/components/ExpensiTextInput/BaseExpensiTextInput.js

src/styles/styles.js

parasharrajat · 2021-11-29T18:13:28Z

src/components/ExpensiTextInput/BaseExpensiTextInput.js

-                                onPressOut={this.props.onPress}
-                                translateX={this.props.translateX}
-                            />
+                            <View style={[styles.flexRow]}>


Move the zIndex: -1 to this View From the Textinput Style and remove it from Textinput

App/src/styles/styles.js

Line 633 in 85edddd

zIndex: -1,

. Otherwise Above mentioned issue....

parasharrajat · 2021-11-29T18:14:35Z

src/components/ExpensiTextInput/BaseExpensiTextInput.js

@@ -167,7 +175,7 @@ class BaseExpensiTextInput extends Component {
                            {hasLabel ? (
                                <>
                                    {/* Adding this background to the label only for multiline text input,
-                                    to prevent text overlaping with label when scrolling */}
+                                    to prevent text overlapping with label when scrolling */}
                                    {this.props.multiline && <View style={styles.expensiTextInputLabelBackground} />}


Could you please add pointerEvents="none" to this View. it fixes one bug, not from your PR.

expensiTextInputLabelBackground: { position: 'absolute', top: 0, width: '100%', height: 25, backgroundColor: themeColors.componentBG, borderTopRightRadius: variables.componentBorderRadiusNormal, borderTopLeftRadius: variables.componentBorderRadiusNormal, pointerEvents: 'none', },

I think pointerEvents=none only work as a prop.

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

anthony-hull · 2021-11-29T18:57:14Z

There is only a bug remaining and a few tweaks needed for the code.

I won't bother much so I have shared the fix in the review comments.

Also other pointer events bug

anthony-hull · 2021-11-29T19:23:11Z

@parasharrajat can you please check again?

parasharrajat · 2021-11-29T19:29:56Z

src/styles/styles.js

@@ -610,17 +613,21 @@ const styles = {
        backgroundColor: themeColors.componentBG,
        borderTopRightRadius: variables.componentBorderRadiusNormal,
        borderTopLeftRadius: variables.componentBorderRadiusNormal,
+        pointerEvents: 'none',


I don't think this works as a style. It is passed as a prop. Right? 😕

You're right there are some platform specifics in implementation when I looked it up. I need to use the prop for it to work.

parasharrajat

LGTM. Thanks for the changes and patience.
All yours @deetergp @johnmlee101

anthony-hull · 2021-11-29T20:01:47Z

No worries! thank you for your patience too!

anthony-hull · 2021-11-30T12:56:41Z

merged your changes @parasharrajat
my screenshots look different to those in your PR

Should the autofill background only be on the input? I quite like how this looks.

But it does introduce a contrast issue for accessibility. The label doesn't contrast enough against the background.

parasharrajat · 2021-11-30T13:01:16Z

Sorry, which changes are you referring to? My PR is yet to be merged and If it merged before yours then you will have some conflicts for sure 😈

But it does introduce a contrast issue for accessibility. The label doesn't contrast enough against the background.

We are not worried about A11y issues ATM but tracking them. Thanks for pointing it out.

anthony-hull · 2021-11-30T13:05:02Z

#4632 nvm! I didn't do a deep enough look. Just saw the screenshot on this was different to what I saw.

parasharrajat · 2021-11-30T13:15:46Z

Oh about that. We have moved away from that change. That PR was about fixing autofill logic not the UI.

anthony-hull · 2021-11-30T14:25:08Z

Looks like it is ok still. Can you re-review?

johnmlee101 · 2021-11-30T19:20:25Z

@deetergp going to skip your review since this was a duplicate (to fix signed commits). just FYI!

OSBotify · 2021-11-30T19:21:28Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-12-07T00:21:38Z

🚀 Deployed to staging by @johnmlee101 in version: 1.1.17-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-12-08T20:13:27Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mananjadhav · 2023-04-27T09:45:16Z

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

add show password toggle to secureTextInput

c96e703

anthony-hull requested a review from a team as a code owner November 29, 2021 16:56

MelvinBot requested review from deetergp and removed request for a team November 29, 2021 16:56

anthony-hull mentioned this pull request Nov 29, 2021

Anthony toggle show password #4746

Closed

5 tasks

johnmlee101 assigned anthony-hull Nov 29, 2021

johnmlee101 previously approved these changes Nov 29, 2021

View reviewed changes

parasharrajat suggested changes Nov 29, 2021

View reviewed changes

move padding right to use utility function

ff42609

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

anthony-hull dismissed johnmlee101’s stale review via ff42609 November 29, 2021 18:20

fix stacking contexts bug

ca44dfb

Also other pointer events bug

anthony-hull requested a review from parasharrajat November 29, 2021 19:25

parasharrajat suggested changes Nov 29, 2021

View reviewed changes

move pointer events to props

bc26dfc

anthony-hull requested a review from parasharrajat November 29, 2021 19:56

parasharrajat previously approved these changes Nov 29, 2021

View reviewed changes

anthony-hull requested a review from johnmlee101 November 29, 2021 20:01

merge main

44b1789

anthony-hull dismissed parasharrajat’s stale review via 44b1789 November 30, 2021 12:47

anthony-hull requested a review from parasharrajat November 30, 2021 14:25

parasharrajat approved these changes Nov 30, 2021

View reviewed changes

johnmlee101 approved these changes Nov 30, 2021

View reviewed changes

johnmlee101 merged commit c9f4292 into Expensify:main Nov 30, 2021

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

kavimuru mentioned this pull request Dec 7, 2021

Web - Settings - Pressing tab on password field won't go straight to next one #6618

Closed

alexxxwork mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add show password toggle to secureTextInput #6501

add show password toggle to secureTextInput #6501

anthony-hull commented Nov 29, 2021 •

edited

Loading

parasharrajat commented Nov 29, 2021

parasharrajat commented Nov 29, 2021

anthony-hull commented Nov 29, 2021

johnmlee101 left a comment

parasharrajat left a comment •

edited

Loading

parasharrajat Nov 29, 2021 •

edited

Loading

parasharrajat Nov 29, 2021

anthony-hull Nov 29, 2021

parasharrajat Nov 29, 2021 •

edited

Loading

anthony-hull commented Nov 29, 2021

anthony-hull commented Nov 29, 2021

parasharrajat Nov 29, 2021

anthony-hull Nov 29, 2021

parasharrajat left a comment •

edited

Loading

anthony-hull commented Nov 29, 2021

anthony-hull commented Nov 30, 2021 •

edited

Loading

parasharrajat commented Nov 30, 2021 •

edited

Loading

anthony-hull commented Nov 30, 2021

parasharrajat commented Nov 30, 2021

anthony-hull commented Nov 30, 2021

johnmlee101 commented Nov 30, 2021

OSBotify commented Nov 30, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

mananjadhav commented Apr 27, 2023

add show password toggle to secureTextInput #6501

add show password toggle to secureTextInput #6501

Conversation

anthony-hull commented Nov 29, 2021 • edited Loading

Details

Fixed Issues

QA Steps

Tested On

Screenshots

Edge

Mobile Web

Desktop

iOS

Android

parasharrajat commented Nov 29, 2021

parasharrajat commented Nov 29, 2021

anthony-hull commented Nov 29, 2021

johnmlee101 left a comment

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

parasharrajat Nov 29, 2021

Choose a reason for hiding this comment

anthony-hull Nov 29, 2021

Choose a reason for hiding this comment

parasharrajat Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

anthony-hull commented Nov 29, 2021

anthony-hull commented Nov 29, 2021

parasharrajat Nov 29, 2021

Choose a reason for hiding this comment

anthony-hull Nov 29, 2021

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

anthony-hull commented Nov 29, 2021

anthony-hull commented Nov 30, 2021 • edited Loading

parasharrajat commented Nov 30, 2021 • edited Loading

anthony-hull commented Nov 30, 2021

parasharrajat commented Nov 30, 2021

anthony-hull commented Nov 30, 2021

johnmlee101 commented Nov 30, 2021

OSBotify commented Nov 30, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

mananjadhav commented Apr 27, 2023

anthony-hull commented Nov 29, 2021 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Nov 29, 2021 •

edited

Loading

parasharrajat Nov 29, 2021 •

edited

Loading

parasharrajat left a comment •

edited

Loading

anthony-hull commented Nov 30, 2021 •

edited

Loading

parasharrajat commented Nov 30, 2021 •

edited

Loading