Disc 2758 enhance text input2

[antoniodangond]

Jira: DISC-2758

Overview:

Modifies TextInput2 error validation behavior:

• Changes initialIsInError prop (unused) to errorOnEmpty
• If errorOnEmpty, empty error state is displayed while input is out of focus and value is empty, but is removed when input is in focus and value is empty
  • The error removal while value is empty allows normal error validation to take place
• Only displays errorValidation error on "blur" event
  • Once the event state is set, error is removed either by correcting the error or removing the value (no error state will be set while the value is empty)

Screenshots/GIFs:

When errorOnEmpty is false:

• error is not shown while value is empty
• after entering invalid value, error is not shown until focus is removed from the input
• error is removed by removing the value
• error is removed by correcting the error (i.e. entering a valid value)
  

When errorOnEmpty is true:

• error is displayed while input is out of focus and value is empty
• empty error is removed on focus
• entering a value allows normal error validation behavior to occur (as described above)
  

Testing:

Note: Added tests use Enzyme's mount function instead of shallow, because useEffect calls aren't made during shallow renders.

• Unit tests
• Manual tests:
  • Chrome
  • Safari
  • IE11

Roll Out:

• Before merging:
  • Updated docs
  • Bumped version in package.json
    • Breaking change?
      • If it is a beta component run npm version minor
      • If the component is not in beta run npm version major
    • New component or backward-compatible component feature change? Run npm version minor
    • Only changing documentation? All good. Skip this step.
  • After creating a new component, make sure to add it to the Components List in ComponentsView.jsx. To do so:
    • Add a screenshot of the component in docs/assets/img with the format <COMPONENT URL LINK>.png
• After merging:
  • Deployed updated docs (make deploy-docs)
  • Posted in #eng if I made a breaking change to a beta component

[antoniodangond]

This change is backwards compatible, since existing implementations of TextInput2 won't provide the showErrorOnBlurOnly prop- hence, !showErrorOnBlurOnly will evaluate to true for those component implementations.

[antoniodangond]

Same backwards compatibility comment as above^

[johnhuangclever]

Nice PR Antonio! Looks like you put alot of thought into the problems with this component and your changes to solve them. I wouldn't worry about backwards compatibility too much here, as I can't imagine anyone would rely on the live error message functionality for anything. Let me know if you see it differently.

I didn't put too much thought into the initial implementation of the error messaging, and this PR has made me do some reading. I found some nice articles and looked at some component libraries:
https://baymard.com/blog/inline-form-validation
https://cxl.com/blog/form-validation/
https://stripe.dev/elements-examples/

What I understand here is that we probably want some of these principles:

• Errors should show only on blur, they clear instantly if they are no longer in error
• If the input has no text entered, it should never show an error
• The REQUIRED error should probably come via a prop and not be internal to the component
  • We can use initialIsInError to handle REQUIRED errors that come from an event. e.g. clicking submit triggering an error state. It doesn't look like this is being used anywhere right now, so potential to rename to requirementError or something
• onBlur and onFocus prop callbacks are good to have in general. However, if we wanted to force the user into input formatting, then rather than using onBlur we could probably add two function props (naming is hard)
  • formatValue: before onChange, call this function then pass the resulting string into onChange. e.g. no trailing/leading spaces, only numbers, auto-case, limit number of characters, etc.
  • formatDisplay: if we wanted other text in the input, we can change how the text is formatted. e.g. phone numbers (XXX) XXX-XXXX, MM/YYYY dates, etc.

Again, I don't think preserving backwards compatibility is too critical. Let me know your thoughts. Thanks!

[johnhuangclever]

Two personal opinions about dirty states:

• I think in general the dirty state is something I think we can avoid, because I don't think dirty states are intuitive to the user. e.g. "I've typed something on the keyboard even if nothing is filled in the input" vs "input is filled, and I'm done with the input. tell me if it's valid"
• If we must have a dirty state, I'd rather have the dirty state be calculated by comparing the initial value with the current state, e.g. https://github.com/Clever/sd2/blob/dff23a6adece60718a228043d5e510290e5f4f1e/ui/pages/IdentityManagement/LoginCredentials/LoginCredentials.tsx#L116-L121

But overall, I think if we are able to sucessfully rewrite the entire validation hook then we should be able to do away with this

[johnhuangclever]

After reading about some best practices for error messaging, we can probably rewrite this entire thing. Something like this maybe:

useEffect(() => {
  // if input is empty and requirement error is active, then do nothing and return
  // if input is empty, then clear error and return


  // check error validation function
  // if no error validation, clear error and return
  // if errormessage exists and is not focused, then set error and return
});

[johnhuangclever]

Nice tests! I think if we add the errorOnEmpty prop to the useEffect dependency list, we can simplify a little bit

[johnhuangclever]

Since this uses shouldDisplayEmptyError, it needs to have errorOnEmpty here as well. I think once that happens we can remove the other useEffect, and let a single useEffect handle everything. e.g.

const [errorMessage, setErrorMessage] = useState(null);

...
// useEffect
if (!value) {
  setErrorMessage(errorOnEmpty && !isFocused ? "" : null);
  return;
}

...
if (!newErrorMessage) {
  setErrorMessage(null);
  return;
}

Correct me if i'm wrong here

[antoniodangond]

Good catch! 2733131

[johnhuangclever]

🙌 Let me know how the implementation goes! Thanks for all your help here and humoring my comments

[antoniodangond]

The implementation seems to be working how we want it! I updated the GIFs in the PR description to show the new implementation. Thanks again for the thorough reviews.