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

crisbeto · 2017-07-30T08:53:05Z

Allows for the md-select error behavior to be configured through an @Input, as well as globally through the same provider as md-input-container.
Simplifies the signature of some of the error option symbols.

jelbourn · 2017-07-31T19:41:46Z

@kara and @mmalerba please take a look

mmalerba · 2017-07-31T20:20:14Z

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

@@ -7,27 +7,24 @@
 */

 import {InjectionToken} from '@angular/core';
-import {FormControl, FormGroupDirective, NgForm} from '@angular/forms';
+import {FormGroupDirective, NgForm, NgControl} from '@angular/forms';

 /** 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');

 export type ErrorStateMatcher =


So I was thinking about maybe refactoring some of this error state matcher stuff because @g1shin recently had to implement a context-sensitive one for the stepper (#6117) and it felt a bit messier than it needed to be... What do you think about something like this:

export interface ErrorStateMatcher { isErrorState(...): boolean; } export class DefaultErrorStateMatcher { isErrorState(...) { ... } } export class ShowOnDirtyErrorStateMatcher { isErrorState(...) { ... } }

And then in error module:

@NgModule({ providers: [{provide: ErrorStateMatcher, useClass: DefaultErrorStateMatcher}] })

And finally in input/select/etc:

constructor(errorStateMatcher: ErrorStateMatcher) { console.log(errorStateMatcher.isErrorState(...)); }

I like this because:

We don't have to worry about the function being ripped off the class and losing its context

We don't have to worry about falling back to the defaultErrorStateMatcher in every component since we provide that in the error module

The code to inject in the constructor doesn't need a whole bunch of @Optional() @Inject(...) stuff

SGTM, but because the matcher is really just the function, it would look more along the lines of this:

export class ErrorOptions { isErrorState = (control, form) => control.invalid && (form.submitted || control.dirty); // more error-related config goes here in the future. }

Also I'm not sure whether we should provide the ShowOnDirtyErrorStateMatcher by default since it's pretty basic logic that people can write themselves if they're overriding it anyway.

+1 Is there other stuff we plan to put in ErrorOptions? Just curious, I can't remember why we went with that over just a function

We don't have anything atm, but the intention is to avoid having to add any more providers if we decide to have more options.

@crisbeto I added it only because it seemed to be a common question here and on SO #4750 (comment), #4750 (comment)

@mmalerba None that I was aware of. Simply based it off of PlaceholderOptions which seemed a nicely future proof

Ok sounds good, I do want to keep the interface though so that components like the stepper can implement it.

interface ErrorOptions { ... } class DefaultErrorOptions implements ErrorOptions { ... }

edit: @crisbeto I'd like to keep the show-on-dirty one because as @willshowell it was a pretty common request and it's nice to have a really easy way for people to do it.

Sure, although FWIW, you can implement a class as well.

Waaa I didn't know that, TS you crazy

crisbeto · 2017-08-02T18:55:20Z

@mmalerba I ended up splitting the ErrorOptions and MdError directive into a separate module to make the DI a little simpler and because we'll probably end up needing the MdError for MdSelect at some point anyway. Can you take another look?

mmalerba · 2017-08-02T19:03:20Z

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

+ */
+@Injectable()
+export class ErrorOptions {
+  errorStateMatcher: ErrorStateMatcher = defaultErrorStateMatcher;


Can we do this method style instead? I want it to be obvious that it's safe to use this in the method

mmalerba · 2017-08-02T19:09:31Z

src/lib/input/input-container.ts

-    const control = this._ngControl;
-    const parent = this._parentFormGroup || this._parentForm;
-    const newState = control && this.errorStateMatcher(control.control as FormControl, parent);
+    const errorMatcher = this.errorStateMatcher || this._errorOptions.errorStateMatcher;


This is bad because it loses the error options context. Honestly, I think we should just get rid of ErrorOptions since there are currently no other options. and make ErrorStateMatcher a class rather than a function. This was we can easily support overriding via @Input like we want to do here and the stepper can easily provide its context-dependent matcher

I'm not sure how making the ErrorStateMatcher would help, considering that it would still look something like:

class ErrorStateMatcher { match(): boolean; }

Otherwise I'll fix it so it doesn't lose the context.

Then instead of making the Input() take a function you make it take a ErrorStateMatcher. My other reason for not wanting ErrorOptions is that things like the stepper which just cares about the matcher then has to worry about all the other options too.

Alright, let's just go back to the injection token with a plain function and we can worry about a provider if we really need it.

crisbeto · 2017-08-02T21:07:22Z

Updated again @mmalerba.

mmalerba

Thanks for the changes, I think this will make it a lot cleaner when we go to do the ErrorStateMatcher for the stepper :)

mmalerba · 2017-08-02T21:26:32Z

src/lib/input/input.md

-A global error state matcher can be specified by setting the `MD_ERROR_GLOBAL_OPTIONS` provider. This applies
-to all inputs. For convenience, `showOnDirtyErrorStateMatcher` is available in order to globally cause
-input errors to show when the input is dirty and invalid.
+A global error state matcher can be specified by setting the `ErrorOptions` provider. This applies


ErrorStateMatcher

mmalerba · 2017-08-02T21:27:26Z

src/lib/input/input.md


 ```ts
 @NgModule({
  providers: [
-    {provide: MD_ERROR_GLOBAL_OPTIONS, useValue: { errorStateMatcher: showOnDirtyErrorStateMatcher }}
+    {provide: ErrorOptions, useClass: ShowOnDirtyErrorStateMatcher}


ErrorStateMatcher

mmalerba · 2017-08-02T21:28:05Z

src/lib/select/select.spec.ts

+    });
+
+    it('should be able to override the error matching behavior via the injection token', () => {
+      const errorOptions: ErrorStateMatcher = {


errorStateMatcher

mmalerba · 2017-08-02T21:31:23Z

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

+/** Error state matcher that matches when a control is invalid and dirty. */
+@Injectable()
+export class ShowOnDirtyErrorStateMatcher implements ErrorStateMatcher {
+  match(control: NgControl | null, form: FormGroupDirective | NgForm | null): boolean {


can we call it isErrorSate? match is kind of generic and might conflict with some other interface someone is implementing

crisbeto · 2017-08-03T05:36:47Z

Addressed the feedback @mmalerba.

kara · 2017-08-03T18:11:53Z

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

+/** Error state matcher that matches when a control is invalid and dirty. */
+@Injectable()
+export class ShowOnDirtyErrorStateMatcher implements ErrorStateMatcher {
+  isErrorSate(control: NgControl | null, form: FormGroupDirective | NgForm | null): boolean {


This public API has a typo - isErrorSate -> isErrorState. Should be fixed across the PR.

Ops, good catch.

* Allows for the `md-select` error behavior to be configured through an `@Input`, as well as globally through the same provider as `md-input-container`. * Simplifies the signature of some of the error option symbols.

mmalerba · 2017-08-07T22:20:08Z

@jelbourn would you rather move md-error into core/error/ or form-field/ as #6331 currently does?

jelbourn · 2017-08-08T18:51:33Z

@mmalerba form-field is good

crisbeto · 2017-08-08T18:54:24Z

In that case let's defer this PR until the form field gets in.

mmalerba · 2017-09-26T16:05:58Z

I think we can resume work on this one now

* Allows for the select's error state matcher to be overwritten through an `@Input`. * Switches `MatSelect` over to use the same global provider for its error state as `MatInput`. **Note:** This is a resubmit of angular#6147 that works with our latest setup and excludes a few changes.

crisbeto · 2017-09-30T12:45:58Z

@mmalerba a lot changed since we parked this PR and it'll take a while to rebase it. I've re-done it, excluded the mat-error changes and resubmitted as #7443.

* Allows for the select's error state matcher to be overwritten through an `@Input`. * Switches `MatSelect` over to use the same global provider for its error state as `MatInput`. **Note:** This is a resubmit of angular#6147 that works with our latest setup and excludes a few changes.

* Allows for the select's error state matcher to be overwritten through an `@Input`. * Switches `MatSelect` over to use the same global provider for its error state as `MatInput`. **Note:** This is a resubmit of angular#6147 that works with our latest setup and excludes a few changes. Fixes angular#7419.

angular-automatic-lock-bot · 2019-09-07T07:02:37Z

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.}

crisbeto added the pr: needs review label Jul 30, 2017

crisbeto assigned jelbourn Jul 30, 2017

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

jelbourn assigned kara and mmalerba Jul 31, 2017

mmalerba reviewed Jul 31, 2017

View reviewed changes

crisbeto force-pushed the select-error-state-matcher branch from a4e2e19 to 062ceb9 Compare August 2, 2017 18:54

crisbeto requested a review from kara as a code owner August 2, 2017 18:54

mmalerba reviewed Aug 2, 2017

View reviewed changes

crisbeto force-pushed the select-error-state-matcher branch from 6d7b7e7 to 2481c3f Compare August 2, 2017 19:32

mmalerba reviewed Aug 2, 2017

View reviewed changes

crisbeto force-pushed the select-error-state-matcher branch from 125155d to 5c6b0a4 Compare August 3, 2017 15:22

mmalerba approved these changes Aug 3, 2017

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Aug 3, 2017

kara removed pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 3, 2017

kara reviewed Aug 3, 2017

View reviewed changes

kara added the pr: needs cleanup / tests label Aug 3, 2017

kara assigned crisbeto and unassigned jelbourn, kara and mmalerba Aug 3, 2017

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 3, 2017

crisbeto removed the pr: needs cleanup / tests label Aug 3, 2017

mmalerba mentioned this pull request Aug 3, 2017

feat(stepper): Add support for linear stepper #6116

Merged

crisbeto added 6 commits August 5, 2017 09:49

feat(select): add support for custom errorStateMatcher

86de634

* Allows for the `md-select` error behavior to be configured through an `@Input`, as well as globally through the same provider as `md-input-container`. * Simplifies the signature of some of the error option symbols.

refactor: split error component into module

fe6e3d2

refactor: preserve context when invoking the error state matcher

bbca4cb

refactor: address feedback

55fb514

refactor: rename match to isErrorState

9200c6f

chore: many typos

32e5de8

crisbeto force-pushed the select-error-state-matcher branch from 16df824 to 32e5de8 Compare August 5, 2017 07:51

fix: update selector in unit test

6ca1928

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Aug 7, 2017

willshowell mentioned this pull request Aug 23, 2017

ErrorStateMatcher for checkbox and slide-toggle #6591

Closed

mmalerba added pr: needs rebase and removed pr: lgtm labels Sep 26, 2017

crisbeto mentioned this pull request Sep 30, 2017

feat(select): add support for custom error state matcher #7443

Merged

crisbeto closed this Sep 30, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

crisbeto commented Jul 30, 2017

jelbourn commented Jul 31, 2017

mmalerba Jul 31, 2017

crisbeto Jul 31, 2017

mmalerba Jul 31, 2017

crisbeto Jul 31, 2017

willshowell Jul 31, 2017

mmalerba Jul 31, 2017 •

edited

Loading

crisbeto Jul 31, 2017

mmalerba Jul 31, 2017

crisbeto commented Aug 2, 2017 •

edited

Loading

mmalerba Aug 2, 2017

crisbeto Aug 2, 2017

mmalerba Aug 2, 2017

crisbeto Aug 2, 2017

mmalerba Aug 2, 2017

crisbeto Aug 2, 2017

crisbeto commented Aug 2, 2017

mmalerba left a comment

mmalerba Aug 2, 2017

mmalerba Aug 2, 2017

mmalerba Aug 2, 2017

mmalerba Aug 2, 2017

crisbeto commented Aug 3, 2017

kara Aug 3, 2017

crisbeto Aug 3, 2017

mmalerba commented Aug 7, 2017 •

edited

Loading

jelbourn commented Aug 8, 2017

crisbeto commented Aug 8, 2017

mmalerba commented Sep 26, 2017

crisbeto commented Sep 30, 2017 •

edited

Loading

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

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

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

Conversation

crisbeto commented Jul 30, 2017

jelbourn commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Aug 2, 2017

mmalerba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Aug 7, 2017 • edited Loading

jelbourn commented Aug 8, 2017

crisbeto commented Aug 8, 2017

mmalerba commented Sep 26, 2017

crisbeto commented Sep 30, 2017 • edited Loading

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

mmalerba Jul 31, 2017 •

edited

Loading

crisbeto commented Aug 2, 2017 •

edited

Loading

mmalerba commented Aug 7, 2017 •

edited

Loading

crisbeto commented Sep 30, 2017 •

edited

Loading