feat(input): add custom error state matcher #4750

willshowell · 2017-05-23T14:52:07Z

Fixes #4232

cc @mmalerba

jelbourn · 2017-05-23T16:34:14Z

I'm wondering if this should be a more generally configurable concept; <md-select> will really need the same thing.

jelbourn · 2017-05-23T16:34:52Z

@kara should also weigh in as this touches on forms

willshowell · 2017-05-23T16:45:06Z

@jelbourn agreed on making it more general. #4672 (or at least #2498) would help

willshowell · 2017-06-09T23:41:03Z

@jelbourn since you last reviewed, I've made the following changes:

errorStateMatcher is a property on the input rather than the container (not ideal for reusability but necessary since fix(input): set aria-invalid on mdInput element #4757)
A global error state matcher can be provided via MD_ERROR_GLOBAL_OPTIONS
Can also configure all inputs to show errors when dirty instead of touched via MD_ERROR_GLOBAL_OPTIONS (since this seems to be a more common request Allow to customize when md-error is shown #4232 (comment), md-hint add color property #2965 (comment))

I'm far from married to # 3, but wanted to explore other global options. Also if this isn't the right approach, that's fine, but I'd like to keep the conversation moving.

mmalerba · 2017-06-21T20:39:03Z

src/lib/core/error/error-options.ts

+export const MD_ERROR_GLOBAL_OPTIONS =
+  new InjectionToken<() => boolean>('md-error-global-options');
+
+export type ErrorStateMatcherType =


just ErrorStateMatcher is fine

mmalerba · 2017-06-21T20:39:25Z

src/lib/core/error/error-options.ts

+
+/** Injection token that can be used to specify the global error options. */
+export const MD_ERROR_GLOBAL_OPTIONS =
+  new InjectionToken<() => boolean>('md-error-global-options');


generic type should be ErrorOptions

mmalerba · 2017-06-21T20:46:58Z

src/lib/input/input-container.spec.ts

    }));

+    it('should display an error message when a custom error matcher returns true', async(() => {
+      fixture.destroy();


can we just put this in a different describe block or something? its weird that we destroy the original fixture here

mmalerba · 2017-06-21T20:56:06Z

src/lib/core/error/error-options.ts

+
+export interface ErrorOptions {
+  errorStateMatcher?: ErrorStateMatcherType;
+  showOnDirty?: boolean;


I feel like this is structured kind of weirdly, how about if we remove the showOnDirty option and instead just add a couple implementations that the user can choose from (e.g. DefaultErrorStateMatcher, ShowOnDirtyErrorStateMatcher)

mmalerba · 2017-06-21T20:56:33Z

src/lib/input/input-container.spec.ts

+
+      customFixture.whenStable().then(() => {
+        expect(containerEl.querySelectorAll('md-error').length)
+          .toBe(0, 'Expected no error messages after being touched.');


nit: indent continuations by 4

mmalerba · 2017-06-21T20:58:27Z

src/lib/input/input-container.ts

    this.id = this.id;
+
+    this._errorOptions = errorOptions ? errorOptions : {};
+    this.errorStateMatcher = this._errorOptions.errorStateMatcher || undefined;


if you create the DefaultErrorStateMatcher class I suggested earlier this can be || new DefaultErrorStateMatcher()

willshowell · 2017-06-22T17:17:59Z

@mmalerba I've addressed your comments! Please advise if I misinterpreted any of your class implementation suggestions.

mmalerba · 2017-06-22T17:29:14Z

src/lib/core/error/error-options.ts

+
+/** Injection token that can be used to specify the global error options. */
+export const MD_ERROR_GLOBAL_OPTIONS =
+  new InjectionToken<ErrorOptions>('md-error-global-options');


nit: I think this will fit on 1 line

mmalerba · 2017-06-22T17:39:24Z

src/lib/core/error/error-options.ts

+  errorStateMatcher?: ErrorStateMatcher;
+}
+
+export class DefaultErrorStateMatcher {


oh sorry, I don't know why I said class, I just meant functions defaultErrorStateMatcher

mmalerba · 2017-06-22T17:39:44Z

src/lib/input/input-container.spec.ts

+@Component({
+  template: `
+    <form #form="ngForm" novalidate>
+      <md-input-container>


nit: indent continuation by 4

willshowell · 2017-06-22T19:33:55Z

@mmalerba done!

The convenience function showOnDirtyErrorStateMatcher will show errors when,

isInvalid && (isDirty || isSubmitted)

But I'm curious if the expected behavior might be,

isInvalid && (isDirty || isTouched || isSubmitted)

kara · 2017-06-26T18:59:15Z

src/demo-app/input/input-demo.ts

    }
  }
+
+  customErrorStateMatcher(c: NgControl): boolean {


In this case, it's a bit odd to pass through the directive because any relevant properties just fall through directly to its FormControl. I think here it would make sense to expose a FormControl instance.

kara · 2017-06-26T19:05:12Z

src/lib/input/input-container.ts

-        (this._parentForm && this._parentForm.submitted);
-
-    return !!(isInvalid && (isTouched || isSubmitted));
+    return this.errorStateMatcher(control, this._parentFormGroup, this._parentForm);


I think we should be passing through this._ngControl.control here. In other words, the FormControl instance rather than the NgControl directive. I don't think most people are familiar with NgControls, and probably aren't familiar with the subtle distinction between the directive and the FormControl itself. While most properties are mirrored on the directive, a few aren't, and I foresee that being confusing.

kara · 2017-06-26T19:10:41Z

src/lib/core/error/error-options.ts

+}
+
+export function showOnDirtyErrorStateMatcher(control: NgControl, formGroup: FormGroupDirective,
+    form: NgForm): boolean {


I don't think we should be surfacing both FormGroupDirective and NgForm here. They are mutually exclusive, so one of these args will always be null. We shouldn't put it on the user to check for the existence of one or the other every time. Given that they both implement the Form interface, it might make more sense to use the Form type here and only pass through the existing parent to this function.

I agree with the sentiment, but I'm not clear on how it would work. The Form interface doesn't have the submitted property, which is the main (if not only) reason we need it. I see two solutions

Use the union FormGroupDirective | NgForm

export function showOnDirtyErrorStateMatcher(control: FormControl, form: FormGroupDirective | NgForm) { const isInvalid = control.invalid; const isDirty = control.dirty; const isSubmitted = form && form.submitted; return !!(isInvalid && (isDirty || isSubmitted)); }

Just pass submitted and don't expose either FormGroupDirective or NgForm

export function showOnDirtyErrorStateMatcher(control: FormControl, submitted: boolean) { const isInvalid = control.invalid; const isDirty = control.dirty; return !!(isInvalid && (isDirty || submitted)); }

Hmm, the fact that the submitted property is missing from Form interface seems like an oversight (will look into it). Given that fact, I prefer #1.

@kara were you ever able to check into why submitted is missing from the Form interface?

kara · 2017-06-26T19:13:05Z

src/lib/input/input-container.spec.ts

  renderError = true;
 }

+@Component({


Remove novalidate? This is added by default by the NgNoValidate directive in @angular/forms.

kara · 2017-06-26T19:15:52Z

src/lib/input/input-container.spec.ts

+@Component({
+  template: `
+    <form #form="ngForm" novalidate>
+      <md-input-container>


NgForm comes with the FormsModule and formControl comes from the ReactiveFormsModule. It's not really great to mix them, and the point of using formControl is to have a standalone control without a parent.

If you want a more "idiomatic" test case, I'd suggest switching to formGroup on the form tag and then use formControlName rather than formControl.

willshowell · 2017-06-26T22:46:45Z

@kara comments addressed. Could you take another look?

kara

This is looking good, just a few more little things. @mmalerba Are you finished reviewing?

kara · 2017-06-26T22:54:07Z

src/lib/core/error/error-options.ts

+
+export function defaultErrorStateMatcher(control: FormControl, form: FormGroupDirective | NgForm) {
+  const isInvalid = control.invalid;
+  const isTouched = control.touched;


Nit: these const assignments seem to be adding bloat, given how short control.touched already is. I'm thinking it's easier to scan this function if you just inline (same on line 32).

const isSubmitted = form && form.submitted; return !!(control.invalid && (control.touched || isSubmitted));

kara · 2017-06-26T23:03:02Z

src/lib/input/input-container.spec.ts

  });

+  describe('custom error behavior', () => {
+    it('should display an error message when a custom error matcher returns true', async(() => {


Given that we're just using reactive form directives now, these tests don't need to be async. You should be able to safely remove the async and whenStable calls.

kara · 2017-06-26T23:44:11Z

LGTM, waiting on @mmalerba to do a last pass.

mmalerba · 2017-06-27T16:12:47Z

src/lib/core/error/error-options.ts

+  errorStateMatcher?: ErrorStateMatcher;
+}
+
+export function defaultErrorStateMatcher(control: FormControl, form: FormGroupDirective | NgForm) {


mmalerba · 2017-06-27T16:12:52Z

src/lib/core/error/error-options.ts

+  return !!(control.invalid && (control.touched || isSubmitted));
+}
+
+export function showOnDirtyErrorStateMatcher(control: FormControl,


* feat(input): Add custom error state matcher * Address comments * Address comments pt. 2 * Use FormControl and only one of incompatible form options * Remove unnecesary async tests and const declarations * Add jsdoc comments to error state matchers

angular-automatic-lock-bot · 2019-09-07T08:02:11Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 23, 2017

jelbourn added the pr: needs review label May 23, 2017

jelbourn added the needs: discussion Further discussion with the team is needed before proceeding label May 23, 2017

willshowell force-pushed the feat/custom-error-function branch from 56b9846 to 5a270f3 Compare May 24, 2017 12:11

willshowell mentioned this pull request May 25, 2017

Allow to customize when md-error is shown #4232

Closed

willshowell force-pushed the feat/custom-error-function branch from 5a270f3 to f22a77a Compare June 1, 2017 12:10

willshowell force-pushed the feat/custom-error-function branch 2 times, most recently from 1f38a53 to d06612a Compare June 9, 2017 23:23

mmalerba requested changes Jun 21, 2017

View reviewed changes

feat(input): Add custom error state matcher

3ff8d87

willshowell force-pushed the feat/custom-error-function branch from d06612a to 84841d0 Compare June 22, 2017 17:04

Address comments

b0d69cc

willshowell force-pushed the feat/custom-error-function branch from 84841d0 to b0d69cc Compare June 22, 2017 17:16

mmalerba reviewed Jun 22, 2017

View reviewed changes

Address comments pt. 2

ab870d1

kara reviewed Jun 26, 2017

View reviewed changes

kara assigned willshowell Jun 26, 2017

kara added the pr: needs cleanup / tests label Jun 26, 2017

Use FormControl and only one of incompatible form options

9bc13bb

willshowell force-pushed the feat/custom-error-function branch from 83954c5 to 9bc13bb Compare June 26, 2017 22:42

kara reviewed Jun 26, 2017

View reviewed changes

Remove unnecesary async tests and const declarations

6f92cdb

willshowell mentioned this pull request Jun 27, 2017

Apply .mat-input-invalid on empty required fields at pristine #4030

Closed

mmalerba approved these changes Jun 27, 2017

View reviewed changes

kara removed needs: discussion Further discussion with the team is needed before proceeding pr: needs review labels Jun 27, 2017

willshowell mentioned this pull request Jun 27, 2017

mat-form-field does not remove mat-form-field-invalid class on FormGroup reset #4190

Open

Add jsdoc comments to error state matchers

ea477cc

kara added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs cleanup / tests labels Jun 27, 2017

tinayuangao merged commit f73cc97 into angular:master Jun 28, 2017

willshowell deleted the feat/custom-error-function branch June 28, 2017 01:19

willshowell mentioned this pull request Jul 31, 2017

feat(select): add support for custom errorStateMatcher #6147

Closed

willshowell mentioned this pull request Aug 23, 2017

ErrorStateMatcher for checkbox and slide-toggle #6591

Closed

willshowell mentioned this pull request Oct 4, 2017

md-select errors still displayed even after resetting formGroup #7522

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

feat(input): add custom error state matcher #4750

feat(input): add custom error state matcher #4750

Uh oh!

Conversation

willshowell commented May 23, 2017

Uh oh!

jelbourn commented May 23, 2017

Uh oh!

jelbourn commented May 23, 2017

Uh oh!

willshowell commented May 23, 2017

Uh oh!

willshowell commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willshowell commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willshowell commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willshowell commented Jun 26, 2017

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kara commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants