This repository has been archived by the owner. It is now read-only.

docs(cb): Add cookbook for validation #1627

Merged
merged 3 commits into from Aug 31, 2016

Conversation

Projects
None yet
7 participants
@DeborahK
Contributor

DeborahK commented Jun 8, 2016

No description provided.

@googlebot googlebot added the CLA: yes label Jun 8, 2016

<div class="form-group">
<!-- #docregion name-with-error-msg -->
<label for="name">Name</label>
<input type="text" class="form-control"

This comment has been minimized.

@mattdistefano

mattdistefano Jun 8, 2016

Noting this here, but it looks like an issue with most of the inputs - these should have an id with the same value as the corresponding label's for, or be nested within the label.

This comment has been minimized.

@DeborahK

DeborahK Jun 9, 2016

Contributor

I copied this code from the forms example at: https://angular.io/docs/ts/latest/guide/forms.html. I added the id's to my code, but the original form examples will need to be changed as well.

<input type="text" class="form-control"
[(ngModel)]="model.name"
ngControl="name"
[ngClass]="{'required': isRequired('name')}">

This comment has been minimized.

@mattdistefano

mattdistefano Jun 8, 2016

Should render a required attribute too for a11y. [Edit: I should add, unless the Validator is doing this itself - I'm not super familiar w/ it yet.]

This comment has been minimized.

@mattdistefano

mattdistefano Jun 8, 2016

Should render an aria-invalid="true" after validation if errors are present. Again, an a11y fix.

This comment has been minimized.

@mattdistefano

mattdistefano Jun 8, 2016

Should also render an aria-describedby pointing at the error div below when it is shown.

This comment has been minimized.

@mattdistefano

mattdistefano Jun 8, 2016

If possible, I'd be inclined to include the minlength and maxlength attrs here too for the built-in browser behavior. Otherwise this technique isn't actually equivalent to the template-driven approach.

This comment has been minimized.

@DeborahK

DeborahK Jun 9, 2016

Contributor

Glad you found the accessibility issues, but the entire point of model-driven is that the knowledge of the validation is only in the code. That way the validation can be adjusted based on the user or on the state (different validation for add vs edit for example). So we don't want to put the validation attributes into the HTML in the model-driven cases. I'm putting together an issue for Angular Forms to suggest that aria attributes are automatically added to the HTML based on the code.

This comment has been minimized.

@DeborahK

DeborahK Jun 9, 2016

Contributor

Because the validation is defined in the code, it could be changed based on the user or situation. For example, a field may be required for a normal user, but not an admin user. When these validation rules are in the code, there is no easy way to put the validation aria attributes in the HTML and have them work properly. (I supposed property binding could work ... but is messy in this case.)
Just as the dev puts the basic attributes in the HTML but the validation rules in the code, it would make sense for the dev to put the basic aria attributes in the HTML, but the validation aria info would be defined in the code. Angular should then add the appropriate validation and aria attributes to the HTML automatically.

This comment has been minimized.

@AlmeroSteyn

AlmeroSteyn Jun 9, 2016

Contributor

We have only three ARIA attributes to consider in most scenarios if developers use the native form elements.

aria-required: If we use HTML5 then the required attribute is enough so we only need to add this if we want to be seriously backwards compatible.

aria-invalid: True or false based on the current validity. Which can easily be sourced from the control object.

Sure, these two can be added by Angular. But we still miss one.

aria-describedby: needs to contain, at any moment, the ID or list of IDs of any visible form validation error message elements. But we also need to edit this list if we want to put in, say, a descriptor field. And say we want to hide the descriptor field when the error messages are visible this has to also be possible. I dunno how reasonable it is to let Angular do this as we will need to provide something like a map of generated/hardcoded field ID's linked to validation errors and have an ngClass type of function to add and remove IDs to the ones already there. Having IDs in this list that are not present on the page, even if it is hidden, creates accessibility violations, so it has to be dynamic.

Example:

<input required aria-describedby="decriptor requiredError">
<span id="descriptor">some description</span>
<span id="requiredError">I am required</span>

Given this, I still would think that it should be Angular's job to add the actual validation attributes to the form control, like maxlength, and required and pattern and the developers job to add the two (maybe three) aria attributes.

Unless it is super easy for them to solve the ID linkup of aria-describedby otherwise there will be the situation where only half of the ARIA attrs get added and the dev has to do the other one and manage it.

This comment has been minimized.

@DeborahK

DeborahK Jun 9, 2016

Contributor

Can we move this discussion to the issue I am posting for Angular 2 Forms? That way the person implementing this issue has all of this detailed and helpful information.

This comment has been minimized.

@AlmeroSteyn

AlmeroSteyn Jun 9, 2016

Contributor

Of course!

This comment has been minimized.

@DeborahK

DeborahK Jun 9, 2016

Contributor

Please add comments and feedback regarding attributes here: angular/angular#9121
Thanks!

## Model-Driven with Data Binding
Using the model-driven approach to form validation, the validation rules are specified in the model as defined in the
component class. Defining the validation in the class instead of the template gives you more control. You

This comment has been minimized.

@mattdistefano

mattdistefano Jun 8, 2016

Does this technique produce rendered HTML equivalent to the template-driven solution? If not, I think that needs to be addressed in some way - either by updating the code so both techniques produce the same HTML or mentioning it in here somewhere.

@mattdistefano

This comment has been minimized.

mattdistefano commented Jun 8, 2016

Added some a11y comments. A lot of these are just issues in the existing TOH code, so I'm not sure if they should be addressed as new cookbooks are added or cleaned up later. Thoughts? CC @AlmeroSteyn @wardbell

- We call this `buildForm` method from the constructor. But in many applications, you may want to call `buildForm`
from somewhere else such as the `ngInit` lifecycle hook.
We'll use the class form and control properties in the template.

This comment has been minimized.

@Foxandxss

Foxandxss Jun 9, 2016

Member

Could you reword this sentence?

This comment has been minimized.

@DeborahK

DeborahK Jun 9, 2016

Contributor

Which sentence are you referring to? Is there something marked in the code above?

This comment has been minimized.

@Foxandxss

Foxandxss Jun 9, 2016

Member

When you add a note like this, it always refers to the immediately line in the top, in this case We'll use the class form and control properties in the template.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 9, 2016

I reviewed it.

A good thing we do in the cookbooks is to add a "Learn more at XXX" with certain things.

I missed it for two topics: What's dirty and what's touched. That explanation of why we want it is good, but a "learn more" and link to the appropriate part of the form guide would be lovely.

Same happens with "controls, controlGroup", a learn more would be nice.

On the other hand, you mention twice that "template-driven" can't be unit tested. Why? I haven't try but it should be posible by changing some values, triggering changes and checking the if model has errors.

You mention that we may call buildForm at ngOnInit. I would do that directly. It is just 2-3 lines of code and sounds better :)

Last, please remove the underscore from private variables (and update the prose too).

Good work, I learn a lot there.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Jun 9, 2016

How is it best to reply to your changes?

  1. I added the learn xxx for dirty and touched.
  2. I could not find anywhere else that we really covered control and controlgroup since we don't have a cookbook or anything on model-driven forms. (I had suggested that this cookbook cover forms and validation ... but the recommendation was to focus on just the validation.) Do you know of something that is just in an unpublished branch?
@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 9, 2016

I don't really know what's covered on the other forms tutorial. If that is not covered (and the new form changes still use that stuff), I think we should do something to fix that.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Jun 9, 2016

I agree ... but not here since the focus here is on the validation ... not the forms.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 9, 2016

Yes, that is outside the scope of this cookbook.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Jun 9, 2016

How important is the change to use ngInit? I didn't use it here because I wanted to keep this simple and focused on validation ... not on explaining how to implement an interface and handle lifecycle hooks.

I removed the underscore and all mentions to unit testing.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 9, 2016

We recommend in other guides to leave constructor to initialize variables and the "heavy" stuff for ngOnInit.

I don't think you need to explain that, just say that we use buildForm at ngOnInit and you are good to go. Same thing as you have now but changing "constructor" for that hook.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Jun 9, 2016

OK.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Jun 9, 2016

That works for me Deborah.

Let see what @wardbell has to say.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Jun 9, 2016

Excellent! Just hoping this makes it into the library before Tuesday. I'm presenting on Forms and Validation in Boston and would love to point people here for more info.

@naomiblack

This comment has been minimized.

Member

naomiblack commented Jun 17, 2016

@DeborahK sorry this didn't make it in for your Boston talk... could you have a look at updating this for the new forms? Also @StephenFluin for help with a final review while Ward is out.

I'm happy to push live when it is.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Jun 17, 2016

@naomiblack I would really love to do this ... especially since I've spent some time with Kara going through the changes. But I am heading to Japan this afternoon and am not taking a computer with me. So I won't be able to do this until I get back at the end of the month.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Aug 26, 2016

@wardbell I updated this for RC 5 and the new forms. It ready for review/merge.

@googlebot

This comment has been minimized.

googlebot commented Aug 27, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment
@googlebot

This comment has been minimized.

googlebot commented Aug 27, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added CLA: no and removed CLA: yes labels Aug 27, 2016

@wardbell

This comment has been minimized.

Contributor

wardbell commented Aug 27, 2016

Rebased and squashed. Tweaking some of the prose and samples.

The most important things I did were

  • rename "validation" to "form-validation". Folders and references adjusted accordingly.
  • rename "model" to "reactive" in labels and files
  • tighten the reactive TS code for readability

More prose fixes to come.

@wardbell

This comment has been minimized.

Contributor

wardbell commented Aug 29, 2016

Putting this on hold until key architectural questions are answered

I really enjoyed reviewing this cookbook. It brought some nagging questions into sharper relief.

The more I looked at it, the more I felt that we remain uncomfortably unclear on the ReactiveForms value proposition.

Frankly, I fail to see anything particularly reactive in the Reactive form approach. We need some insight here that perhaps @kara can supply.

I dispelled the myth that the template approach involves more, ugly HTML. It seemed to me that myth arises from primitive examples rather than the template approach per se.

Yes, the primitive examples bake error logic and messages into the HTML as we see in the first example. But this is not an intrinsic defect of the template approach. In my last commit I showed an intermediate Template 2 form that _uses the same validation message code as in the Reactive form example_.

I aligned code and html for Template 1, Template 2, and Reactive 3 examples such that the reader can see the progression and true differences.

I am troubled by the claim that the reactive approach is easier to test. There isn't any evidence in support of the notion in this example (although there is an opening for a testability claim in my comments below).

AFAIK, the substantive differences between template and reactive approaches are:

  1. Reactive defines the form controls in code within the component class; layout and styling remain in html.

  2. Reactive defines the validation functions in code within the component class rather than as HTML validation attributes.

My intuition is that developers will find #2 to be the most significant. It also creates an architectural dilemma.

OTOH, having validation in code rather than attributes has advantages:

  • richer validation logic is possible
  • the validation logic could be re-factored into the model (or model metadata) rather than hard-coded in the component.
  • the validation logic can be tested independently of the component's interaction with its template, especially if refactored into the model.

OTOH, the glaring negative is lack of ARIA support. It's easy but tedious to add that support manually (I updated the sample with a hint in that direction). Ideally the Angular library can do more to be helpful.

@wardbell

This comment has been minimized.

Contributor

wardbell commented Aug 30, 2016

Talked to Kara and got some additional clarification on required and ARIA.

I removed the isRequired function which is tough to talk about and not recommended by her. She says we should keep the required attribute and add Validator.required. Do both. We'll only need the validator function in a future version of forms; it will add the required attribute for us.

DeborahK and others added some commits Jun 1, 2016

docs(cb-form-validation): ward's tweaks
renamed from cb-validation to cb-form-validation
other refactorings and text changes
@wardbell

This comment has been minimized.

Contributor

wardbell commented Aug 31, 2016

Added a custom validation ... as that is something you can do easily with Reactive Forms and can't do AFAIK with the Template-driven.

At this point the cookbook has evolved rather considerably from the inspirational version by @DeborahK .

Ready for a review.

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Aug 31, 2016

@wardbell I had a custom validator ... but removed it to keep the chapter short. You can use a custom validator with template-driven. You just need to wrap it with a directive. I have an example here if you want to take a look: https://github.com/DeborahK/VSLive2016-Anaheim/blob/master/angular2-Forms/MovieHunter%20-%20Forms/app/movies/movie-edit.component.html

@DeborahK

This comment has been minimized.

Contributor

DeborahK commented Aug 31, 2016

I thought a separate cookbook on custom validation might be useful. It could cover some specific cases that people often ask about ... such as cross-field validation (such as a start and end date) and async validation (such as checking that a userName already exists).

@wardbell

This comment has been minimized.

Contributor

wardbell commented Aug 31, 2016

I just learned about that from @kara.

Now I'm torn. I think it is important to see where the true differences are w/r/t validation in the two approaches. I thought custom validation might have been one of them. It would have been a shame not to show it.

Now that I know there is symmetry (if not parity) on that front, I'm not sure what to do. Pulling custom validation out seems to undersell the story.

I agree there is more to cover ... cross-field, async, who knows what else. Can't do it all in here.

Thinking.

docs(cb-form-validation): add template2 - a step between template and…
… reactive

Raises questions about what really separates Forms from ReactiveForms
@wardbell

This comment has been minimized.

Contributor

wardbell commented Aug 31, 2016

Decided that we should show custom validation so that readers know it is possible. By putting the validation logic in its own file (in the shared folder), we postpone discussion to the bottom of the cookbook so that it doesn't distract from the main story.

I think this works.

Enough already. Merging it.

@wardbell wardbell merged commit b2ce694 into angular:master Aug 31, 2016

0 of 2 checks passed

cla/google CLAs are signed, but unable to verify author consent
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.