Skip to content

Components: Forms and validation #7614

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

Merged
merged 70 commits into from
Feb 20, 2019
Merged

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Feb 15, 2019

OK, this is a pretty massive PR, so @rynowak / @javiercn / @pranavkm - would any of you like me to give you a 30-minute tour of the changes and new features? Or if you prefer just to dive into the code that's completely fine with me too.

Features

  • EditContext - this tracks metadata about an edit process (e.g., what's been modified, current validation messages, etc.). It doesn't do any actual validation itself, but rather has an API designed to let external systems hook into it. External things can get notification about when fields are edited so they can choose to validate if they want.
  • EditForm - this creates an EditContext for you and cascades it down to contents. It also provides extra events (OnValidSubmit, OnInvalidSubmit, though you can still use OnSubmit directly if you want to trigger the validation yourself based on your own lifecycle design).
  • DataAnnotationsValidator - this attaches data annotations support to an EditContext
  • ValidationSummary - like the one in MVC
  • Built-in input components (InputText, InputCheckbox, InputSelect, etc.). These provide default behavior of validating on edit and changing their CSS class to reflect field state. Some of them have useful parsing logic (e.g., InputDate and InputNumber handle unparseable values gracefully by registering them as validation errors). The relevant ones also support nullability of the target field (e.g., int?).

For usage examples, see the new test cases in the E2E BasicTestApp:

  • SimpleValidationComponent
  • TypicalValidationComponent
  • NotifyPropertyChangedValidationComponent

Still to do for preview 3

  • Make the build pass. Need to check with @natemcmaster what's needed to deal with the platform manifest error. Should be OK now.
  • Rebase on master. This is tricky because in the last few days, the repo stopped taking a dependency on DataAnnotations, so I need to put that back. And first I need approval to join the darc group, etc. Done
  • E2E tests. The test cases themselves are implemented - I just haven't written the Selenium automation for them yet. Done
  • Maybe implement a ValidationMessage component, which would be trivial. Or developers can implement it themseles. By design, these sorts of things don't require priviledged access to any internal APIs. Implemented.
  • Remove the manual usages of *Expression from the test cases once this enhancement gets merged into Razor: Components that accept bind-Something can request SomethingExpression razor#213 Not waiting for this.

Further enhancements planned for after preview 3

  • Async validation. I have a design in mind, but there's only so much can get done in time for Preview 3, and it would be good to get some customer feedback before trying to do everything.
  • Maybe some way to configure a default validation system, so you don't have to put <DataAnnotationsValidator> in every EditForm if that's what you're using.
  • Maybe some way to override the default CSS classes on the built-in Input* components
  • Ideally (though nontrivial), support for splatting attributes on Razor Components. Then we could make the built-in input controls automatically pass through all extra attributes, and developers who subclass them can still get that attribute passthrough behavior. Until we do something like this, some basic use cases like setting a maxlength on InputText just aren't supported (or at least, you have to subclass InputText and put that feature on your subclass, which is inconvenient).

@SteveSandersonMS
Copy link
Member Author

@natemcmaster Is it OK for me to put back the dependency on System.ComponentModel.Annotations that was removed from this repo in the last few days? We are still depending on it transitively via Microsoft.Extensions.Options, so it seems like there's no reason why it shouldn't be referenced from shared framework assemblies. If I understand correctly, I need to use darc to do this, somehow.

@natemcmaster
Copy link
Contributor

@SteveSandersonMS See https://github.com/aspnet/AspNetCore/blob/master/docs/ReferenceResolution.md#example-adding-a-new-dependency.

I removed it because projects don't need this <Reference> if they target netcoreapp3.0. But it looks like this PR adds the reference in a netstandard2.0 project, so yes, a direct dependency on System.ComponentModel.Annotations is okay to put back.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 15, 2019
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/forms-and-validation branch from e8397d2 to 3cc40e4 Compare February 19, 2019 21:16
@SteveSandersonMS
Copy link
Member Author

@natemcmaster Changed as requested. Please leave a 5☆ review if satisfactory :)

Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

⭐️ ⭐️ ⭐️ ⭐️ ⭐️

@SteveSandersonMS
Copy link
Member Author

/AzurePipelines run all

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@SteveSandersonMS
Copy link
Member Author

/AzurePipelines Go home, you're drunk.

@azure-pipelines
Copy link

Command 'Go' is not supported by Azure Pipelines.

Supported commands
     help:
          Get descriptions, examples and documentation about supported commands
          Example: help "command_name"
     run:
          Run all pipelines or a specific pipeline for this repository using a comment. Use
          this command by itself to trigger all related pipelines, or specify a pipeline
          to run.
          Example: "run" or "run pipeline_name"

See additional documentation.

@rynowak
Copy link
Member

rynowak commented Feb 19, 2019

Command 'Go' is not supported by Azure Pipelines.

Supported commands
help:
Get descriptions, examples and documentation about supported commands
Example: help "command_name"
run:
Run all pipelines or a specific pipeline for this repository using a comment. Use
this command by itself to trigger all related pipelines, or specify a pipeline
to run.
Example: "run" or "run pipeline_name"

See additional documentation.

boom

@SteveSandersonMS SteveSandersonMS merged commit 7a2dfd3 into master Feb 20, 2019
@SteveSandersonMS SteveSandersonMS deleted the stevesa/forms-and-validation branch February 20, 2019 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants