[HOLD for payment 2022-11-18] [$500] Standardize form error styles

[shawnborton]

Problem:
Right now we have mutliple different ways of styling a form error.

Solution:
Let's standardize on how we style these form errors. Ideally this should follow the red brick road (dot indicator) style.

Details:

• Let's standardize on the dot indicator component we have as the main way to show an error
• Let's update this component to use all red text (except things like links) instead of gray text

[melvin-bot]

Current assignee @shawnborton is eligible for the Design assigner, not assigning anyone new.

[shawnborton]

After some brief Slack discussion, let's move forward with making the error text all red as well. I have updated the original comment in this issue with the desired outcome.

[melvin-bot]

Triggered auto assignment to @dylanexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[Puneet-here]

Proposal

We can reuse DotIndicatorMessage here, we will also need to add a new prop textStyle we will use it for passing styles for text here

App/src/components/DotIndicatorMessage.js

Line 61 in 124ab42

<Text key={i} style={styles.offlineFeedback.text}>{message}</Text>

And we just have to make changes in BaseTextInput

 class BaseTextInput extends Component {
     constructor(props) {
@@ -198,7 +199,6 @@ class BaseTextInput extends Component {
         const inputProps = _.omit(this.props, _.keys(baseTextInputPropTypes.propTypes));
         const hasLabel = Boolean(this.props.label.length);
         const inputHelpText = this.props.errorText || this.props.hint;
-        const formHelpStyles = this.props.errorText ? styles.formError : styles.formHelp;
         const placeholder = (this.props.prefixCharacter || this.state.isFocused || !hasLabel || (hasLabel && this.props.forceActiveLabel)) ? this.props.placeholder : null;
         const textInputContainerStyles = _.reduce([
             styles.textInputContainer,
@@ -304,10 +304,13 @@ class BaseTextInput extends Component {
                             </View>
                         </TouchableWithoutFeedback>
                     </View>
-                    {!_.isEmpty(inputHelpText) && (
-                        <Text style={[formHelpStyles, styles.mt1, styles.ph3]}>
-                            {inputHelpText}
-                        </Text>
+                    {(!_.isEmpty(this.props.hint) && _.isEmpty(this.props.errorText)) && (
+                    <Text style={[styles.formHelp, styles.mt1, styles.ph3]}>
+                        {inputHelpText}
+                    </Text>
+                    )}
+                    {!_.isEmpty(this.props.errorText) && (
+                        <DotIndicatorMessage messages={{0: inputHelpText}} type="error" textStyle={[styles.formError, styles.mt1]} />
                     )}
                 </View>

We can also use a variable shouldShowHint to check if we can show the hint and we can use it like this

                    {shouldShowHint && (
                        <Text style={[formHelpStyles, styles.mt1, styles.ph3]}>
                            {inputHelpText}
                        </Text>
                    )}

For bottom error we need to pass themeColors.textError at

App/src/components/FormAlertWrapper.js

Line 63 in 124ab42

{!_.isEmpty(props.message) && !props.isMessageHtml && <Text style={styles.mutedTextLabel}>{props.message}</Text>}

App/src/components/FormAlertWrapper.js

Line 67 in 124ab42

<Text style={styles.mutedTextLabel}>

App/src/components/FormAlertWrapper.js

Line 76 in 124ab42

<Text style={styles.mutedTextLabel}>

and for icon we can use -

<Icon src={Expensicons.DotIndicator} fill={colors.red} height={variables.iconSizeSmall} width={variables.iconSizeSmall} />

App/src/components/FormAlertWrapper.js

Line 59 in 124ab42

<Icon src={Expensicons.Exclamation} fill={colors.red} />

FormAlertWrapper component will be refactored soon with the red dot in #11410 so I think we should leave this part to get fixed there.

[dylanexpensify]

Reviewing now!

[parasharrajat]

No bot message. Interesting!

[parasharrajat]

@Puneet-here either customize the DotIndicatorMessage completely to fit here or create a new simple component for the same.

To me, this issue is exported first and completely focuses on the styling and mentions it in the requirements so it should be done as part of it.

[dylanexpensify]

Upwork: https://www.upwork.com/jobs/~01f4b150900a44d8e0

[thienlnam]

Going to be OOO next week so I'm going to re-assign so this can keep moving along

[melvin-bot]

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Current assignee @parasharrajat is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[mountiny]

@parasharrajat @Puneet-here thanks, waiting for a full proposal

[mountiny]

I think let just do it here. @0xmiroslav Could you please raise a follow up PR to fix this as well?

I agree, that should be simple enough. No need to CP either.

[parasharrajat]

Yeah, no CP needed. Just a addon PR.

[dylanexpensify]

Do we have an ETA when we could get the add-on PR raised?

[0xmiros]

I think we missed one location in the app to check. I did during testing as well. (its hard to remember all pages)
the PasswordForm for protected PDF attachment.

This page is not using Form.js. That's why we missed it. And yes, it's ideal to standardize all non-form error styles as well by using new FormHelpMessage component.

I raised PR. I also fixed other issues related this page, needed to test fixes and take screenshots for PR.

cc: @parasharrajat @mountiny

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• The PR that introduced the bug has been identified. Link to the PR:
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• A discussion in #contributor-plus has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.26-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix issues in pdf password form #12547

If no regressions arise, payment will be issued on 2022-11-18. 🎊

[mountiny]

@dylanexpensify lets not forget to increase the bounty here for $750 given @0xmiroslav had to work on this one a bit more. Not sure how you keep track of these, maybe update a title

[dylanexpensify]

reviewing today!

[dylanexpensify]

@parasharrajat @0xmiroslav please apply here! TY!

[0xmiros]

@dylanexpensify applied thanks

[dylanexpensify]

Offers sent! @parasharrajat @0xmiroslav

[mountiny]

@0xmiroslav @parasharrajat Has this been all paid up?

[0xmiros]

@0xmiroslav @parasharrajat Has this been all paid up?

not yet

[NicMendonca]

@parasharrajat @0xmiroslav paid! (keeping this open for the rest of the checklist)

[mountiny]

I think this was not a bug per se but more so a follow up to our Form refactor and aligning on the styles in other forms/inputs hence I am not linking any PR since this was not a regression.

And therefore we dont have to add any new test into testrail either given this is not regression and there are tests covering forms and failure.s

[parasharrajat]

I think I was not paid the correct amount as per #11908 (comment). @NicMendonca

[dylanexpensify]

@parasharrajat sent over amount missing!