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

[$250] Ensure we only have one error message show for forms that have only one input #55649

Open
2 tasks
dylanexpensify opened this issue Jan 23, 2025 · 31 comments
Open
2 tasks
Assignees
Labels
Daily KSv2 External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Overdue

Comments

@dylanexpensify
Copy link
Contributor

dylanexpensify commented Jan 23, 2025

Problem: moving into a newDot world, we've been able to make a lot of our forms shorter and with fewer inputs per form. When users make an input that isn't the correct format for the form an error is thrown both in the input and at the bottom of the form. When they click on the hyperlink to fix the error, it autofocuses them to the input in question. This can result in confusion though for a user when multiple error messages show for a form that has only one input.

Solution: remove footer errors when only one input exists in a form. This removes any confusion for a user who might click on the hyperlink and be brought to the only input.

Next steps:

  • Review all forms we have in product for NewDot
  • For all that have only one input, remove the footer error message

Mocks
Image

cc @Expensify/design

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021885281718152248847
  • Upwork Job ID: 1885281718152248847
  • Last Price Increase: 2025-02-07
  • Automatic offers:
    • shubham1206agra | Contributor | 106076444
Issue OwnerCurrent Issue Owner: @shubham1206agra
@dylanexpensify dylanexpensify added Improvement Item broken or needs improvement. Task labels Jan 23, 2025
@dylanexpensify dylanexpensify self-assigned this Jan 23, 2025
@dylanexpensify
Copy link
Contributor Author

Coming from this internal slack post for visibility for internal members.

@shubham1206agra
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Ensure we only have one error message show for forms that have only one input

What is the root cause of that problem?

Improvement Task

What changes do you think we should make in order to solve the problem?

  • Review all forms we have in product for NewDot

For this, we will review all the usage of FormProvider component and determine whether we should show or hide the Fix alert

  • For all that have only one input, remove the footer error message

After doing the previous step, we will use pre-existing prop named shouldHideFixErrorsAlert, which can be put on FormProvider component.

One example of existing usage -

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA since this is an UI change

What alternative solutions did you explore? (Optional)

@dannymcclain
Copy link
Contributor

Added a mock to OP to make it a little more clear what the expected outcome would look like.

Copy link

melvin-bot bot commented Jan 29, 2025

@dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@dylanexpensify
Copy link
Contributor Author

NICE! Thanks @dannymcclain!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 29, 2025
@dylanexpensify dylanexpensify added the External Added to denote the issue can be worked on by a contributor label Jan 31, 2025
@melvin-bot melvin-bot bot changed the title Ensure we only have one error message show for forms that have only one input [$250] Ensure we only have one error message show for forms that have only one input Jan 31, 2025
Copy link

melvin-bot bot commented Jan 31, 2025

Job added to Upwork: https://www.upwork.com/jobs/~021885281718152248847

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 31, 2025
Copy link

melvin-bot bot commented Jan 31, 2025

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rojiphil (External)

Copy link

melvin-bot bot commented Feb 4, 2025

@rojiphil, @dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Feb 4, 2025
@rojiphil
Copy link
Contributor

rojiphil commented Feb 4, 2025

Will review today

@melvin-bot melvin-bot bot removed the Overdue label Feb 4, 2025
@rojiphil
Copy link
Contributor

rojiphil commented Feb 4, 2025

@shubham1206agra proposal to use shouldHideFixErrorsAlert for all forms having only one input should solve this issue. Proposal LGTM.
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 4, 2025

Triggered auto assignment to @roryabraham, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@dylanexpensify
Copy link
Contributor Author

Nice! Rory to confirm

@dylanexpensify
Copy link
Contributor Author

bump le @roryabraham 🙇‍♂

@melvin-bot melvin-bot bot added the Overdue label Feb 6, 2025
Copy link

melvin-bot bot commented Feb 7, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@dylanexpensify
Copy link
Contributor Author

Hackathon is today, so Rory will get to this early next week

Copy link

melvin-bot bot commented Feb 10, 2025

@rojiphil, @roryabraham, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

@roryabraham
Copy link
Contributor

roryabraham commented Feb 11, 2025

Apologies for the delay. I notice a very consistent pattern with existing usages of shouldHideFixErrorsAlert - it is only ever passed as true if there is exactly one InputWrapper component within the parent FormProvider. and in this issue, we're doing an audit to try and ensure that all forms that have exactly one InputWrapper component have shouldHideFixErrorsAlert=true.

Rather than having this prop that should always be true if and only if there's exactly one InputWrapper component within the FormProvider, let's just look at that to determine whether we show the fix errors alert or not, then remove the prop from the codebase:

diff
diff --git a/src/components/Form/FormWrapper.tsx b/src/components/Form/FormWrapper.tsx
index 43fc9cd7af7..a9d7fbf9b9e 100644
--- a/src/components/Form/FormWrapper.tsx
+++ b/src/components/Form/FormWrapper.tsx
@@ -57,7 +57,6 @@ function FormWrapper({
     formID,
     shouldUseScrollView = true,
     scrollContextEnabled = false,
-    shouldHideFixErrorsAlert = false,
     disablePressOnEnter = false,
     isSubmitDisabled = false,
     isLoading = false,
@@ -102,8 +101,11 @@ function FormWrapper({
         focusInput?.focus?.();
     }, [errors, formState?.errorFields, inputRefs]);
 
-    const scrollViewContent = useCallback(
-        () => (
+    const scrollViewContent = useCallback(() => {
+        // We show the fix errors alert if an only if there's exactly one top-level child of this FormWrapper that's an InputWrapper
+        // In other words, it shows only if the form has more than one input, because if there's only one input than the error at the bottom is redundant
+        const shouldHideFixErrorsAlert = React.Children.toArray(children).filter((child) => React.isValidElement(child) && child.type === 'InputWrapper').length === 1;
+        return (
             <FormElement
                 key={formID}
                 ref={formContentRef}
@@ -129,35 +131,33 @@ function FormWrapper({
                     />
                 )}
             </FormElement>
-        ),
-        [
-            formID,
-            style,
-            safeAreaInsetPaddingBottom,
-            styles.pb5.paddingBottom,
-            styles.mh0,
-            styles.mt5,
-            styles.flex1,
-            children,
-            isSubmitButtonVisible,
-            submitButtonText,
-            isSubmitDisabled,
-            errors,
-            formState?.errorFields,
-            formState?.isLoading,
-            shouldHideFixErrorsAlert,
-            errorMessage,
-            isLoading,
-            onSubmit,
-            footerContent,
-            onFixTheErrorsLinkPressed,
-            submitFlexEnabled,
-            submitButtonStyles,
-            enabledWhenOffline,
-            isSubmitActionDangerous,
-            disablePressOnEnter,
-        ],
-    );
+        );
+    }, [
+        formID,
+        style,
+        safeAreaInsetPaddingBottom,
+        styles.pb5.paddingBottom,
+        styles.mh0,
+        styles.mt5,
+        styles.flex1,
+        children,
+        isSubmitButtonVisible,
+        submitButtonText,
+        isSubmitDisabled,
+        errors,
+        formState?.errorFields,
+        formState?.isLoading,
+        errorMessage,
+        isLoading,
+        onSubmit,
+        footerContent,
+        onFixTheErrorsLinkPressed,
+        submitFlexEnabled,
+        submitButtonStyles,
+        enabledWhenOffline,
+        isSubmitActionDangerous,
+        disablePressOnEnter,
+    ]);
 
     if (!shouldUseScrollView) {
         return scrollViewContent();
diff --git a/src/components/Form/types.ts b/src/components/Form/types.ts
index 02cc4e899b3..98a8ac295f7 100644
--- a/src/components/Form/types.ts
+++ b/src/components/Form/types.ts
@@ -146,9 +146,6 @@ type FormProps<TFormID extends OnyxFormKey = OnyxFormKey> = {
     /** Whether the form submit action is dangerous */
     isSubmitActionDangerous?: boolean;
 
-    /** Should fix the errors alert be displayed when there is an error in the form */
-    shouldHideFixErrorsAlert?: boolean;
-
     /** Whether ScrollWithContext should be used instead of regular ScrollView. Set to true when there's a nested Picker component in Form. */
     scrollContextEnabled?: boolean;
 
diff --git a/src/pages/EnablePayments/IdologyQuestions.tsx b/src/pages/EnablePayments/IdologyQuestions.tsx
index 602754bef55..01c5f6a3d6c 100644
--- a/src/pages/EnablePayments/IdologyQuestions.tsx
+++ b/src/pages/EnablePayments/IdologyQuestions.tsx
@@ -117,7 +117,6 @@ function IdologyQuestions({questions, idNumber}: IdologyQuestionsProps) {
                 scrollContextEnabled
                 style={[styles.flexGrow1, styles.ph5]}
                 submitButtonText={translate('common.saveAndContinue')}
-                shouldHideFixErrorsAlert
             >
                 <>
                     <InputWrapper
diff --git a/src/pages/settings/Wallet/InternationalDepositAccount/substeps/Confirmation.tsx b/src/pages/settings/Wallet/InternationalDepositAccount/substeps/Confirmation.tsx
index 38e3de4cf1e..c52e349026e 100644
--- a/src/pages/settings/Wallet/InternationalDepositAccount/substeps/Confirmation.tsx
+++ b/src/pages/settings/Wallet/InternationalDepositAccount/substeps/Confirmation.tsx
@@ -169,7 +169,6 @@ function Confirmation({onNext, onMove, formValues, fieldsMap}: CustomSubStepProp
                 style={[styles.mh5, styles.flexGrow1]}
                 enabledWhenOffline={false}
                 isLoading={isSubmitting}
-                shouldHideFixErrorsAlert
             >
                 <InputWrapper
                     InputComponent={CheckboxWithLabel}
diff --git a/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/ChooseSegmentTypeStep.tsx b/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/ChooseSegmentTypeStep.tsx
index 5077064cf6a..506241d0ec1 100644
--- a/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/ChooseSegmentTypeStep.tsx
+++ b/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/ChooseSegmentTypeStep.tsx
@@ -52,7 +52,6 @@ function ChooseSegmentTypeStep({onNext, setCustomSegmentType, isEditing, netSuit
             submitButtonStyles={[styles.ph5, styles.mb0]}
             enabledWhenOffline
             shouldUseScrollView
-            shouldHideFixErrorsAlert
             submitFlexEnabled={false}
         >
             <Text style={[styles.ph5, styles.textHeadlineLineHeightXXL, styles.mb3]}>
diff --git a/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomListMappingStep.tsx b/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomListMappingStep.tsx
index 6602b83bb1a..3035d3c7c42 100644
--- a/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomListMappingStep.tsx
+++ b/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomListMappingStep.tsx
@@ -56,7 +56,6 @@ function CustomListMappingStep({importCustomField, customSegmentType, onNext, is
             style={[styles.flexGrow1, styles.mt3]}
             submitButtonStyles={[styles.ph5, styles.mb0]}
             enabledWhenOffline
-            shouldHideFixErrorsAlert
             submitFlexEnabled={false}
         >
             <Text style={[styles.ph5, styles.textHeadlineLineHeightXXL, styles.mb3]}>{translate(titleKey as TranslationPaths)}</Text>
diff --git a/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomSegmentMappingStep.tsx b/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomSegmentMappingStep.tsx
index 6686d86850c..ef9f697ed50 100644
--- a/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomSegmentMappingStep.tsx
+++ b/src/pages/workspace/accounting/netsuite/import/NetSuiteImportCustomFieldNew/substeps/CustomSegmentMappingStep.tsx
@@ -56,7 +56,6 @@ function CustomSegmentMappingStep({importCustomField, customSegmentType, onNext,
             submitButtonStyles={[styles.ph5, styles.mb0]}
             enabledWhenOffline
             shouldUseScrollView={false}
-            shouldHideFixErrorsAlert
             submitFlexEnabled={false}
         >
             <Text style={[styles.ph5, styles.textHeadlineLineHeightXXL, styles.mb3]}>{translate(titleKey as TranslationPaths)}</Text>
diff --git a/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx b/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx
index 156f941804e..21a6d64131a 100644
--- a/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx
+++ b/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx
@@ -79,7 +79,6 @@ function CreateDistanceRatePage({route}: CreateDistanceRatePageProps) {
                         validate={validate}
                         enabledWhenOffline
                         style={[styles.flexGrow1]}
-                        shouldHideFixErrorsAlert
                         submitFlexEnabled={false}
                         submitButtonStyles={[styles.mh5, styles.mt0]}
                     >
diff --git a/src/pages/workspace/distanceRates/PolicyDistanceRateEditPage.tsx b/src/pages/workspace/distanceRates/PolicyDistanceRateEditPage.tsx
index 2266a2254d4..726d552dac5 100644
--- a/src/pages/workspace/distanceRates/PolicyDistanceRateEditPage.tsx
+++ b/src/pages/workspace/distanceRates/PolicyDistanceRateEditPage.tsx
@@ -84,7 +84,6 @@ function PolicyDistanceRateEditPage({route}: PolicyDistanceRateEditPageProps) {
                     validate={validate}
                     enabledWhenOffline
                     style={[styles.flexGrow1]}
-                    shouldHideFixErrorsAlert
                     submitFlexEnabled={false}
                     submitButtonStyles={[styles.mh5, styles.mt0]}
                 >
diff --git a/src/pages/workspace/distanceRates/PolicyDistanceRateTaxReclaimableEditPage.tsx b/src/pages/workspace/distanceRates/PolicyDistanceRateTaxReclaimableEditPage.tsx
index fd62217f657..01db82e47ce 100644
--- a/src/pages/workspace/distanceRates/PolicyDistanceRateTaxReclaimableEditPage.tsx
+++ b/src/pages/workspace/distanceRates/PolicyDistanceRateTaxReclaimableEditPage.tsx
@@ -83,7 +83,6 @@ function PolicyDistanceRateTaxReclaimableEditPage({route, policy}: PolicyDistanc
                     validate={validate}
                     enabledWhenOffline
                     style={[styles.flexGrow1]}
-                    shouldHideFixErrorsAlert
                     submitFlexEnabled={false}
                     submitButtonStyles={[styles.mh5, styles.mt0]}
                 >
diff --git a/src/pages/workspace/expensifyCard/WorkspaceEditCardLimitPage.tsx b/src/pages/workspace/expensifyCard/WorkspaceEditCardLimitPage.tsx
index 8ab07e78e35..cfadd231346 100644
--- a/src/pages/workspace/expensifyCard/WorkspaceEditCardLimitPage.tsx
+++ b/src/pages/workspace/expensifyCard/WorkspaceEditCardLimitPage.tsx
@@ -120,7 +120,6 @@ function WorkspaceEditCardLimitPage({route}: WorkspaceEditCardLimitPageProps) {
                 <FormProvider
                     formID={ONYXKEYS.FORMS.EDIT_EXPENSIFY_CARD_LIMIT_FORM}
                     submitButtonText={translate('common.save')}
-                    shouldHideFixErrorsAlert
                     onSubmit={submit}
                     style={styles.flex1}
                     submitButtonStyles={[styles.mh5, styles.mt0]}
diff --git a/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx b/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx
index 84b607bc406..ed9d37473cf 100644
--- a/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx
+++ b/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx
@@ -82,7 +82,6 @@ function LimitStep({policyID}: LimitStepProps) {
             <FormProvider
                 formID={ONYXKEYS.FORMS.ISSUE_NEW_EXPENSIFY_CARD_FORM}
                 submitButtonText={translate(isEditing ? 'common.confirm' : 'common.next')}
-                shouldHideFixErrorsAlert
                 onSubmit={submit}
                 style={[styles.flex1]}
                 submitButtonStyles={[styles.mh5, styles.mt0]}
diff --git a/src/pages/workspace/taxes/ValuePage.tsx b/src/pages/workspace/taxes/ValuePage.tsx
index 8369e9c3501..f07a1ce28a1 100644
--- a/src/pages/workspace/taxes/ValuePage.tsx
+++ b/src/pages/workspace/taxes/ValuePage.tsx
@@ -80,7 +80,6 @@ function ValuePage({
                     validate={validateTaxValue}
                     onSubmit={submit}
                     enabledWhenOffline
-                    shouldHideFixErrorsAlert
                     submitFlexEnabled={false}
                     submitButtonStyles={[styles.mh5, styles.mt0]}
                 >

It would also be great if we could create an automated UI test to cover that logic, rendering just the FormProvider and FormWrapper using react-native-testing-library, with different kinds of children to validate that this works as expected and document the expected behavior.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot removed the Overdue label Feb 11, 2025
@dylanexpensify
Copy link
Contributor Author

ongoing

Copy link

melvin-bot bot commented Feb 14, 2025

@rojiphil, @roryabraham, @shubham1206agra, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Feb 14, 2025
@roryabraham roryabraham removed the Task label Feb 14, 2025
@roryabraham
Copy link
Contributor

@shubham1206agra any update here?

@shubham1206agra
Copy link
Contributor

@roryabraham The approach you defined will not work on multi-step forms since we don't automatically destroy reference so it might create issues.

@melvin-bot melvin-bot bot removed the Overdue label Feb 17, 2025
@roryabraham
Copy link
Contributor

I'm sorry, I don't understand. Can you please explain in more detail what you mean by that?

Copy link

melvin-bot bot commented Feb 21, 2025

@rojiphil, @roryabraham, @shubham1206agra, @dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Feb 21, 2025
Copy link

melvin-bot bot commented Feb 25, 2025

@shubham1206agra Still overdue 6 days?! Let's take care of this!

@shubham1206agra
Copy link
Contributor

Sorry @roryabraham, I was bit under weather.

Actually, I hit a road bump with your approach. There are many places where we don't call InputWrapper directly under FormProvider. Example - https://github.com/Expensify/App/blob/main/src/components/AddressForm.tsx

Which will affect the actual count in your logic. So, I am trying to get around some idea that will use registerInput. If I can't come up with any idea, then I will proceed with manual audit of forms.

Copy link

melvin-bot bot commented Mar 5, 2025

@shubham1206agra Still overdue 6 days?! Let's take care of this!

@melvin-bot melvin-bot bot added the Overdue label Mar 5, 2025
@roryabraham
Copy link
Contributor

post in slack if you get stuck or find yourself spending a lot of time digging into this

Copy link

melvin-bot bot commented Mar 7, 2025

@shubham1206agra 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@shubham1206agra
Copy link
Contributor

Fixing 100 eslint issues.

@melvin-bot melvin-bot bot removed the Overdue label Mar 7, 2025
Copy link

melvin-bot bot commented Mar 10, 2025

@shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Overdue
Projects
None yet
Development

No branches or pull requests

5 participants