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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add isRequired to inputs #1978

Open
andrewleader opened this Issue Oct 1, 2018 · 13 comments

Comments

6 participants
@andrewleader
Copy link
Collaborator

andrewleader commented Oct 1, 2018

Release Renderer status Tasks status
1.2 馃攧 .NET (#2410)
馃攧 Android (#2411)
馃攧 iOS (#2412)
鉁旓笍 TS (#2413)
馃攧 UWP (#2414)
馃攧 Shared (#2409)
鉁旓笍 Designer (#2526)

Solves requests

  • Basic IsRequired validation on inputs #1936

Summary

Add a validation property on Input elements, which currently only has necessity (so you can mark it as required)

Schema

New property on Input elements.

Property Type Required Description
validation InputValidationOptions false Specifies the validation options for the input.

New InputValidationOptions type

Property Type Required Description
necessity InputValidationOptions false Specifies the necessity of the input (whether it's required or optional)
errorMessage string false Specifies the text that will be displayed if validation failed. This is optional but strongly recommended. If not provided, there will be NO error message displayed (the field will simply be visually highlighted as invalid).

In the future, you can imagine we'll add regex to the options.

New InputValidationNecessity enum

Value Description
optional Default. The input is optional.
required The input is required. Actions that require inputs will be blocked until valid input is provided.
requiredWithVisualCue Same as required, except this adds a visual cue to indicate the field is required before the user has even interacted with the card.

New property on Action elements.

Property Type Required Description
ignoreInputValidation bool false Defaults to false. If this is set to true, any input validation will be ignored when the user clicks the button. Useful for cases where you have a "Save" button (that requires inputs) and a "View" button (that doesn't require inputs).

OPEN QUESTION: Should we also allow authors to provide custom text for the validation error? Maybe that's a vNext feature? Or maybe we do it now? It probably shouldn't be required for authors to provide that text though, there's something nice about not needing to provide that text.

Example

{
  "type": "AdaptiveCard",
  "version": "1.0",
  "body": [
    {
      "type": "TextBlock",
      "text": "Please confirm your username"
    },
    {
      "type": "Input.Text",
      "id": "username",
      "placeholder": "Username",
      "validation": {
        "necessity": "required",
        "errorMessage": "Username is required"
      }
    }
  ],
  "actions": [
    {
      "type": "Action.Submit",
      "title": "Confirm",
      "data": "action=confirm"
    },
    {
      "type": "Action.Submit",
      "title": "Cancel",
      "data": "action=cancel",
      "ignoreInputValidation": true
    }
  ]
}

Initial state
image

User clicks Confirm, we show validation errors (and trigger event to host telling host where it needs to scroll to, in case some of the invalid fields were off-screen)
image

User changes the input value, we remove the validation error. We'll re-validate upon button press again. So even if they type "a" and then delete it, the validation error should remain removed.

image

Note that we do NOT place the validation above the input, since it breaks the flow of the header text related to the input. And we can't place the validation above the header text, since inputs don't have a header property.

image

Other inputs

image image

Host Config

No changes. We'll use the attention color for the error text.

Host APIs

Hosts need to have APIs similar to below (in the corresponding native style)

// Disable default drawing of errors since we'll draw our own
renderer.DisplayInputValidationErrors = false; // True by default

// Returns invalid inputs
List<Input> invalidInputs = renderedCard.ValidateInputs();

// Intercept action executing and show different input errors
override void onExecuteAction(Action action)
{
    List<AdaptiveInput> invalidInputs = action.validateInputs();

    foreach (var input in invalidInputs)
    {
        showPopup(input.domElement);
    }
}

// Listen to input value changes so can re-run input validation
override void onInputValueChanged(Input input)
{
    // Clear error popups on inputs that were fixed
}

Down-level impact

Medium. Authors need to expect that users running down-level can still submit the card with empty inputs (most concerning for new authors who might assume that required property works, existing authors should already have that logic).

Host burden

None. Hosts could choose to native style, but they don't have to.

Renderer Requirements

  1. A renderer must respect the required inputs and not allow action execution until the required inputs are fulfilled.
  2. A renderer must visually indicate required fields if necessity is set to requiredWithVisualCue
  3. A renderer should display the validation errors as described in this spec.
  4. If displaying validation errors, a renderer must use the text provided in the errorMessage. If none is specified, that's a parse error (since it's required).

Auto-generated task status

  • Shared
  • Designer
  • .NET
  • Android
  • iOS
  • TS
  • UWP
@mdtauk

This comment has been minimized.

Copy link

mdtauk commented Oct 1, 2018

Didn't the XAML validation changes to the Text Input controls get pushed back to 19H1? Android by default support field validation, but I can't speak for iOS. Do the FluentWeb/Fabric fields support validation states for HTML renderers?

@andrewleader

This comment has been minimized.

Copy link
Collaborator Author

andrewleader commented Oct 1, 2018

We would add validation UI ourselves for platforms that don't have any.

@andrewleader

This comment has been minimized.

Copy link
Collaborator Author

andrewleader commented Oct 2, 2018

Author ability to specify "required" text, and it's mandatory so we don't have to localize

@andrewleader andrewleader changed the title Proposal: Add validation/isRequired to inputs Spec Draft: Add validation/isRequired to inputs Oct 3, 2018

@andrewleader

This comment has been minimized.

Copy link
Collaborator Author

andrewleader commented Oct 3, 2018

Approved, updated with changes from our proposal review for adding the validationFailedText property.

Note that in the proposal review, we discussed the ability to specify the validationFailedText higher up, and let it cascade down to all inputs, but we decided that for now we'll just leave it on individual inputs. If that request comes up in the future, we can always add it.

@andrewleader andrewleader added the Spec label Oct 12, 2018

@andrewleader andrewleader added this to Spec drafts in 1.2 Release Oct 30, 2018

@khouzam khouzam added the Epic label Jan 28, 2019

@vijaysaimutyala

This comment has been minimized.

Copy link

vijaysaimutyala commented Feb 14, 2019

Thanks for the wonderful work team. I understand that asking for a release date is bad, but we're using adaptive cards heavily in one of our bots and input validation is one of the important features we're missing. Do we have any tentative date for the 1.2 release or is any beta url for 1.2 with Input validation that I can test ?

@andrewleader andrewleader changed the title Spec Draft: Add validation/isRequired to inputs Spec Draft: Add isRequired to inputs Feb 14, 2019

@andrewleader

This comment has been minimized.

Copy link
Collaborator Author

andrewleader commented Feb 14, 2019

Thanks @vijaysaimutyala! We still don't have any release date to share, but we've almost implemented all the 1.2 features so we're nearing the end. However, we haven't implemented this feature yet, so there's currently nothing you can try out. But this is a high priority feature (and planned to be part of 1.2), so we'll be working on it soon!

By the way, this feature only allows you to mark a field as required. It does NOT provide support for validating a valid email address, phone number, etc... if you need those features, let us know (and those will be addressed in a separate feature). Thanks!

@vijaysaimutyala

This comment has been minimized.

Copy link

vijaysaimutyala commented Feb 16, 2019

Glad to know this @andrewleader. Marking the field as required will have most of the job done. But client side verification of inputs like email address or Phone Number like you said would be great. I'll try and come up with what all inputs validations will be useful. Thanks.

@dclaux

This comment has been minimized.

Copy link
Member

dclaux commented Mar 8, 2019

As I implement the feature, I would like to suggest a couple changes to the spec:

  • validationFailedText: this is a property of the validation object. Its name therefore doesn't need to start with "validation", just the same as "necessity" wasn't named "validationNecessity". I think we should rename this property into failureMessage
  • I don't think we should require the failureMessage property. I think a built-in default failure message would be enough in case the property is omitted.
  • I think we need to allow hosts to retrieve the list of inputs that fail validation:
    • Introduce a new Action.validateInputs(): Input[] method. The method would return a collection of all inputs that have failed validation
    • Introduce a new onInputValidationFailed(action: Action) event. This event would trigger instead of onExecuteAction when at least one of the inputs fails validation
@dclaux

This comment has been minimized.

Copy link
Member

dclaux commented Mar 8, 2019

One more thing:

  • I think ignoreInputValidation should be a property of Action.Submit (and Action.Http) only, not a property of the base Action type.
@andrewleader

This comment has been minimized.

Copy link
Collaborator Author

andrewleader commented Mar 9, 2019

Good points, I agree with renaming to failureMessage or failedText or errorText (definitely should get rid of the leading "validation").

For the built-in default failure message, our concern is that it means we have to provide localized text for the default failure message. Additionally, we believe that in most cases, developers use custom strings anyways (but that's just a guess, we don't have evidence backing it up). Our main concern would be the localized text.

For hosts retrieving failed input validations, what would be the purpose of this? What's the use case? I'm sure you have one, but we need to know it before it makes sense to add those APIs :)

Agreed about ignoreInputValidation only being on actions that submit data from the inputs (like Submit/Http).

I can start a conversation with the whole team about these points Monday, but some more clarity around why you think we should have the two middle features will help!

@dclaux

This comment has been minimized.

Copy link
Member

dclaux commented Mar 9, 2019

@andrewleader I know about that localization concern given I am part of that collective "we" you reference and helped shape this spec. As I implement it though, I do believe requiring the failure message is overkill. Maybe our default should be to not show an error message if not specified; this eliminates the localization issue.

As for the method/event:

  • The method needs to exist anyway in some form; it would be how the renderer decides to trigger the onExecuteAction event or not. We might as well make that method public.
  • But the actual use case is those hosts that have constrained height cards; there is not space to display the error messages in the card itself within messing up with the overall layout. Such hosts will want to display error messages their own way, say in a popup for instance.
@dclaux

This comment has been minimized.

Copy link
Member

dclaux commented Mar 11, 2019

I suggest we use errorMessage (which is somewhat standard) instead of validationFailedText. That's what I will implement now, and I can always change it if necessary.

@andrewleader

This comment has been minimized.

Copy link
Collaborator Author

andrewleader commented Mar 20, 2019

After consideration, we're making the following updates to the spec...

  • Renaming validationFailedText to errorMessage (because "validation" is redundant since it's already inside a validation object, and error message sounds more accurate).
  • Changing validationFailedText/errorMessage property from required to optional. We will NOT provide a default error message, so we still recommend developers provide an error message, but don't require it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.