Skip to content
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

Fix Inconsistency in the security page and Request money page #23712

Merged
merged 25 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/languages/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ export default {
threadSentMoneyReportName: ({formattedAmount, comment}) => `${formattedAmount} sent${comment ? ` for ${comment}` : ''}`,
requestCount: ({count}) => `${count} requests`,
error: {
invalidAmount: 'Please enter a valid amount before continuing.',
invalidSplit: 'Split amounts do not equal total amount',
other: 'Unexpected error, please try again later',
genericCreateFailureMessage: 'Unexpected error requesting money, please try again later',
Expand Down Expand Up @@ -590,6 +591,7 @@ export default {
keepCodesSafe: 'Keep these recovery codes safe!',
codesLoseAccess:
'If you lose access to your authenticator app and don’t have these codes, you will lose access to your account. \n\nNote: Setting up two-factor authentication will log you out of all other active sessions.',
errorStepCodes: 'Please copy or download codes before continuing.',
stepVerify: 'Verify',
scanCode: 'Scan the QR code using your',
authenticatorApp: 'authenticator app',
Expand Down
2 changes: 2 additions & 0 deletions src/languages/es.js
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,7 @@ export default {
threadSentMoneyReportName: ({formattedAmount, comment}) => `${formattedAmount} enviado${comment ? ` para ${comment}` : ''}`,
requestCount: ({count}) => `${count} solicitudes`,
error: {
invalidAmount: 'Por favor ingresa un monto válido antes de continuar.',
invalidSplit: 'La suma de las partes no equivale al monto total',
other: 'Error inesperado, por favor inténtalo más tarde',
genericCreateFailureMessage: 'Error inesperado solicitando dinero, Por favor, inténtalo más tarde',
Expand Down Expand Up @@ -591,6 +592,7 @@ export default {
keepCodesSafe: '¡Guarda los códigos de recuperación en un lugar seguro!',
codesLoseAccess:
'Si pierdes el acceso a tu aplicación de autenticación y no tienes estos códigos, perderás el acceso a tu cuenta. \n\nNota: Configurar la autenticación de dos factores cerrará la sesión de todas las demás sesiones activas.',
errorStepCodes: 'Copia o descarga los códigos antes de continuar.',
stepVerify: 'Verificar',
scanCode: 'Escanea el código QR usando tu',
authenticatorApp: 'aplicación de autenticación',
Expand Down
35 changes: 26 additions & 9 deletions src/pages/iou/steps/MoneyRequestAmountForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import * as DeviceCapabilities from '../../../libs/DeviceCapabilities';
import TextInputWithCurrencySymbol from '../../../components/TextInputWithCurrencySymbol';
import useLocalize from '../../../hooks/useLocalize';
import CONST from '../../../CONST';
import FormHelpMessage from '../../../components/FormHelpMessage';

const propTypes = {
/** IOU amount saved in Onyx */
Expand Down Expand Up @@ -57,6 +58,8 @@ const getNewSelection = (oldSelection, prevLength, newLength) => {
return {start: cursorPosition, end: cursorPosition};
};

const checkAmount = (amount) => !amount.length || parseFloat(amount) < 0.01;
dukenv0307 marked this conversation as resolved.
Show resolved Hide resolved

const AMOUNT_VIEW_ID = 'amountView';
const NUM_PAD_CONTAINER_VIEW_ID = 'numPadContainerView';
const NUM_PAD_VIEW_ID = 'numPadView';
Expand All @@ -69,6 +72,8 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu
const selectedAmountAsString = amount ? CurrencyUtils.convertToWholeUnit(currency, amount).toString() : '';

const [currentAmount, setCurrentAmount] = useState(selectedAmountAsString);
const [isInvaidAmount, setIsInvalidAmount] = useState(checkAmount(selectedAmountAsString));
dukenv0307 marked this conversation as resolved.
Show resolved Hide resolved
const [formError, setFormError] = useState('');
const [shouldUpdateSelection, setShouldUpdateSelection] = useState(true);

const [selection, setSelection] = useState({
Expand Down Expand Up @@ -124,6 +129,8 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu
setSelection((prevSelection) => ({...prevSelection}));
return;
}
setIsInvalidAmount(checkAmount(newAmountWithoutSpaces));
setFormError('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that maybe in order to be congruent with the Form pages we should only clear the error once the user has input a valid amount. Example:

Screen.Recording.2023-08-21.at.16.27.08.mov

Copy link
Contributor Author

@dukenv0307 dukenv0307 Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ollyws I just updated.

setCurrentAmount((prevAmount) => {
setSelection((prevSelection) => getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length));
return MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);
Expand Down Expand Up @@ -172,8 +179,12 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu
* Submit amount and navigate to a proper page
*/
const submitAndNavigateToNextPage = useCallback(() => {
if (isInvaidAmount) {
setFormError('iou.error.invalidAmount');
return;
}
onSubmitButtonPress(currentAmount);
}, [onSubmitButtonPress, currentAmount]);
}, [onSubmitButtonPress, currentAmount, isInvaidAmount]);

const formattedAmount = MoneyRequestUtils.replaceAllDigits(currentAmount, toLocaleDigit);
const buttonText = isEditing ? translate('common.save') : translate('common.next');
Expand Down Expand Up @@ -222,14 +233,20 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu
longPressHandlerStateChanged={updateLongPressHandlerState}
/>
) : null}
<Button
success
style={[styles.w100, styles.mt5]}
onPress={submitAndNavigateToNextPage}
pressOnEnter
isDisabled={!currentAmount.length || parseFloat(currentAmount) < 0.01}
text={buttonText}
/>
<View style={[styles.w100, styles.mt5]}>
Copy link
Contributor

@Ollyws Ollyws Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we update the alignment to match the other Form pages? Thanks.

Screenshot 2023-08-22 at 15 49 34 Screenshot 2023-08-22 at 15 49 42

Also I'm wondering if we can stop the input from moving up when the error is added, it's a bit jarring:

Screen.Recording.2023-08-22.at.15.57.19.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ollyws I think it's hard to prevent jumping because the input has style flex:1.
@shawnborton I think we should confirm here again. Should we disable the next button of money request flow?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this look like on mobile where we show the full keypad too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like this.

Screenshot 2023-08-22 at 22 50 18

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some UI could be broken as @Ollyws commented here So I think we should only not disable the button in CodesSteps of TwoFactor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should consider moving that error message to be below the green button for consistency. I think it's probably okay if the UI jumps slightly because the alternatives aren't really too great. We'd have to give a an area a fixed height to prevent jumping, which kind of feels like we'd be optimizing for the less common case.

Also, we should make the error text use our text.Supporting color, but keep the dot red.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should consider moving that error message to be below the green button for consistency

What does that mean? I saw the error message is always above the confirm button.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawnborton please help me in the comment above when you have a chance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then that makes an awkward gap between the green button and the keypad. So I think for this one, we should move the error message to be below the green submit button.

{!_.isEmpty(formError) && (
<FormHelpMessage
isError
message={translate(formError)}
/>
)}
<Button
success
onPress={submitAndNavigateToNextPage}
pressOnEnter
text={buttonText}
/>
</View>
</View>
</>
);
Expand Down
19 changes: 17 additions & 2 deletions src/pages/settings/Security/TwoFactorAuth/CodesPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import Clipboard from '../../../../libs/Clipboard';
import themeColors from '../../../../styles/themes/default';
import localFileDownload from '../../../../libs/localFileDownload';
import * as TwoFactorAuthActions from '../../../../libs/actions/TwoFactorAuthActions';
import FormHelpMessage from '../../../../components/FormHelpMessage';

const propTypes = {
...withLocalizePropTypes,
Expand All @@ -46,6 +47,7 @@ const defaultProps = {

function CodesPage(props) {
const [isNextButtonDisabled, setIsNextButtonDisabled] = useState(true);
const [error, setError] = useState('');

// Here, this eslint rule will make the unmount effect unreadable, possibly confusing with mount
// eslint-disable-next-line arrow-body-style
Expand Down Expand Up @@ -106,6 +108,7 @@ function CodesPage(props) {
onPress={() => {
Clipboard.setString(props.account.recoveryCodes);
setIsNextButtonDisabled(false);
setError('');
}}
styles={[styles.button, styles.buttonMedium, styles.twoFactorAuthCodesButton]}
textStyles={[styles.buttonMediumText]}
Expand All @@ -116,6 +119,7 @@ function CodesPage(props) {
onPress={() => {
localFileDownload('two-factor-auth-codes', props.account.recoveryCodes);
setIsNextButtonDisabled(false);
setError('');
}}
inline={false}
styles={[styles.button, styles.buttonMedium, styles.twoFactorAuthCodesButton]}
Expand All @@ -128,11 +132,22 @@ function CodesPage(props) {
</Section>
</ScrollView>
<FixedFooter style={[styles.mtAuto, styles.pt2]}>
{!_.isEmpty(error) && (
<FormHelpMessage
isError
message={props.translate(error)}
/>
)}
<Button
success
text={props.translate('common.next')}
onPress={() => Navigation.navigate(ROUTES.SETTINGS_2FA_VERIFY)}
isDisabled={isNextButtonDisabled}
onPress={() => {
if (isNextButtonDisabled) {
setError('twoFactorAuth.errorStepCodes');
return;
}
Navigation.navigate(ROUTES.SETTINGS_2FA_VERIFY);
}}
/>
</FixedFooter>
</FullPageOfflineBlockingView>
Expand Down
Loading