fix: Change password hint color to red when invalid, else grey #5130
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import React, {Component} from 'react'; | |
| import {View, ScrollView} from 'react-native'; | ||
| import Onyx, {withOnyx} from 'react-native-onyx'; | ||
| import PropTypes from 'prop-types'; | ||
| import _ from 'underscore'; | ||
|
|
||
| import HeaderWithCloseButton from '../../components/HeaderWithCloseButton'; | ||
| import Navigation from '../../libs/Navigation/Navigation'; | ||
|
|
@@ -50,45 +50,76 @@ class PasswordPage extends Component { | |
| currentPassword: '', | ||
| newPassword: '', | ||
| confirmNewPassword: '', | ||
| errors: { | ||
| currentPassword: false, | ||
| newPassword: false, | ||
| confirmNewPassword: false, | ||
| confirmPasswordMatch: false, | ||
| }, | ||
| }; | ||
|
|
||
| this.handleChangePassword = this.handleChangePassword.bind(this); | ||
| this.getErrorText = this.getErrorText.bind(this); | ||
| this.isValidPassword = this.isValidPassword.bind(this); | ||
| this.validateAndSubmitForm = this.validateAndSubmitForm.bind(this); | ||
| this.clearErrorAndSetValue = this.clearErrorAndSetValue.bind(this); | ||
| this.currentPasswordInputRef = null; | ||
|
|
||
| this.errorKeysMap = { | ||
| currentPassword: 'passwordPage.errors.currentPassword', | ||
| confirmNewPassword: 'passwordPage.errors.confirmNewPassword', | ||
| }; | ||
| } | ||
|
|
||
| componentWillUnmount() { | ||
| Onyx.merge(ONYXKEYS.ACCOUNT, {error: '', success: ''}); | ||
| } | ||
|
|
||
| getErrorText(field) { | ||
| if (this.state.errors[field]) { | ||
| return this.props.translate(this.errorKeysMap[field]); | ||
| } | ||
| return null; | ||
|
There was a problem hiding this comment. If the doc says we are returning a string then don't return |
||
| } | ||
|
|
||
| isValidPassword(password) { | ||
| return password.match(CONST.PASSWORD_COMPLEXITY_REGEX_STRING); | ||
| } | ||
|
There was a problem hiding this comment. Can we move this to App/src/libs/ValidationUtils.js Lines 216 to 228 in 34442f4 |
||
|
|
||
| clearErrorAndSetValue(field, value) { | ||
|
There was a problem hiding this comment. Add jsdocs here and to other functions There was a problem hiding this comment. Updated. Sorry, not sure how I missed these 🤔 |
||
| this.setState(prevState => ({ | ||
| [field]: value, | ||
| errors: {...prevState.errors, [field]: false}, | ||
| })); | ||
| } | ||
|
|
||
| validateAndSubmitForm() { | ||
|
There was a problem hiding this comment. Can we please make one function that validates and another that submits? Here's an example: App/src/pages/ReimbursementAccount/CompanyStep.js Lines 173 to 181 in 34442f4 |
||
| const errors = {}; | ||
|
|
||
| if (!this.state.currentPassword) { | ||
| errors.currentPassword = true; | ||
| } | ||
|
|
||
| if (!this.state.newPassword || !this.isValidPassword(this.state.newPassword)) { | ||
| errors.newPassword = true; | ||
| } | ||
|
|
||
| if (!this.state.confirmNewPassword) { | ||
| errors.confirmNewPassword = true; | ||
| } | ||
|
|
||
| if (this.state.currentPassword && this.state.newPassword && this.state.currentPassword === this.state.newPassword) { | ||
| errors.newPassword = true; | ||
| } | ||
|
|
||
| if (this.state.newPassword && this.state.confirmNewPassword && !this.doPasswordsMatch()) { | ||
| errors.confirmPasswordMatch = true; | ||
| } | ||
|
|
||
| this.setState({errors}); | ||
| if (_.isEmpty(errors)) { | ||
| this.handleChangePassword(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -132,8 +163,10 @@ class PasswordPage extends Component { | |
| autoCompleteType="password" | ||
| textContentType="password" | ||
| value={this.state.currentPassword} | ||
| onChangeText={text => this.clearErrorAndSetValue('currentPassword', text)} | ||
| returnKeyType="done" | ||
| hasError={this.state.errors.currentPassword} | ||
| errorText={this.getErrorText('currentPassword')} | ||
| /> | ||
| </View> | ||
| <View style={styles.mb6}> | ||
|
|
@@ -143,15 +176,19 @@ class PasswordPage extends Component { | |
| autoCompleteType="password" | ||
| textContentType="password" | ||
| value={this.state.newPassword} | ||
| hasError={this.state.errors.newPassword} | ||
| errorText="" | ||
|
There was a problem hiding this comment. This is not needed. We should default to an empty string if not passing this value. |
||
| onChangeText={text => this.clearErrorAndSetValue('newPassword', text)} | ||
| /> | ||
| <Text | ||
| style={[ | ||
| styles.textLabelSupporting, | ||
| styles.mt1, | ||
| this.state.errors.newPassword && styles.formError, | ||
| ]} | ||
| > | ||
| {this.props.translate('passwordPage.newPasswordPrompt')} | ||
| </Text> | ||
| </View> | ||
| <View style={styles.mb6}> | ||
| <ExpensiTextInput | ||
|
|
@@ -160,17 +197,18 @@ class PasswordPage extends Component { | |
| autoCompleteType="password" | ||
| textContentType="password" | ||
| value={this.state.confirmNewPassword} | ||
| onChangeText={text => this.clearErrorAndSetValue('confirmNewPassword', text)} | ||
| hasError={this.state.errors.confirmNewPassword} | ||
| errorText={this.getErrorText('confirmNewPassword')} | ||
| onSubmitEditing={this.validateAndSubmitForm} | ||
| /> | ||
| </View> | ||
| {!this.state.errors.confirmPasswordMatch && !_.isEmpty(this.props.account.error) && ( | ||
| <Text style={styles.formError}> | ||
| {this.props.account.error} | ||
| </Text> | ||
| )} | ||
| {this.state.errors.confirmPasswordMatch && ( | ||
|
There was a problem hiding this comment. Should we still be showing this error? It feels kind of out of place and I'm not sure it's correct. If the first password is valid but the confirmation password doesn't match then the password confirmation field is the one with the "error"? Seems like it wouldn't make sense to show this:
There was a problem hiding this comment. I've updated. I was a bit unsure about guidelines on tagging multiple error flags to a single field. I've added an approach. Let me know if this is fine. There was a problem hiding this comment. This also is something I wanted to discuss (perhaps back on the original issue or via Slack) before moving forward on. |
||
| <InlineErrorText> | ||
| {this.props.translate('setPasswordPage.passwordsDontMatch')} | ||
| </InlineErrorText> | ||
|
|
@@ -180,14 +218,9 @@ class PasswordPage extends Component { | |
| <Button | ||
| success | ||
| style={[styles.mb2]} | ||
| isLoading={this.props.account.loading} | ||
| text={this.props.translate('common.save')} | ||
| onPress={this.validateAndSubmitForm} | ||
| /> | ||
| </FixedFooter> | ||
| </KeyboardAvoidingView> | ||
|
|
||
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.
This error message seems wrong. If your problem is that the new password isn't different from the old password then that's what we should say. We're kind of mixing feedback here instead of telling the user exactly what's wrong.
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.
As a user, I'm going to wonder which is my problem? Passwords need to change? Or format is bad?
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.
You're right, but I wasn't sure if this is supposed to be changed. Updated
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.
Apologies, to clarify, mainly wanted to discuss this improvement and get some opinions before updating.