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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jun 28, 2017

Conversation

willshowell
Copy link
Contributor

Fixes #4232

cc @mmalerba

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

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

@jelbourn
Copy link
Member

@kara should also weigh in as this touches on forms

@jelbourn jelbourn added the needs: discussion Further discussion with the team is needed before proceeding label May 23, 2017
@willshowell
Copy link
Contributor Author

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

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

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

  1. 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)
  2. A global error state matcher can be provided via MD_ERROR_GLOBAL_OPTIONS
  3. 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.

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

export type ErrorStateMatcherType =
Copy link
Contributor

Choose a reason for hiding this comment

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

just ErrorStateMatcher is fine


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

Choose a reason for hiding this comment

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

generic type should be ErrorOptions

@@ -706,6 +709,116 @@ describe('MdInputContainer', function () {
});
}));

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

Choose a reason for hiding this comment

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

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


export interface ErrorOptions {
errorStateMatcher?: ErrorStateMatcherType;
showOnDirty?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)


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

Choose a reason for hiding this comment

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

nit: indent continuations by 4


// Force setter to be called in case id was not specified.
this.id = this.id;

this._errorOptions = errorOptions ? errorOptions : {};
this.errorStateMatcher = this._errorOptions.errorStateMatcher || undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@willshowell
Copy link
Contributor Author

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


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

Choose a reason for hiding this comment

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

nit: I think this will fit on 1 line

errorStateMatcher?: ErrorStateMatcher;
}

export class DefaultErrorStateMatcher {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<form #form="ngForm" novalidate>
<md-input-container>
<input mdInput
[formControl]="formControl"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indent continuation by 4

@willshowell
Copy link
Contributor Author

@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)

@@ -43,4 +44,11 @@ export class InputDemo {
this.items.push({ value: ++max });
}
}

customErrorStateMatcher(c: NgControl): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

(this._parentForm && this._parentForm.submitted);

return !!(isInvalid && (isTouched || isSubmitted));
return this.errorStateMatcher(control, this._parentFormGroup, this._parentForm);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

export function showOnDirtyErrorStateMatcher(control: NgControl, formGroup: FormGroupDirective,
form: NgForm): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. 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));
}
  1. 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));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -1020,6 +1136,29 @@ class MdInputContainerWithFormErrorMessages {

@Component({
template: `
<form #form="ngForm" novalidate>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<form #form="ngForm" novalidate>
<md-input-container>
<input mdInput
[formControl]="formControl"
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@kara comments addressed. Could you take another look?

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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));

@@ -749,6 +751,121 @@ describe('MdInputContainer', function () {

});

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor

kara commented Jun 26, 2017

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

errorStateMatcher?: ErrorStateMatcher;
}

export function defaultErrorStateMatcher(control: FormControl, form: FormGroupDirective | NgForm) {
Copy link
Contributor

Choose a reason for hiding this comment

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

docs

return !!(control.invalid && (control.touched || isSubmitted));
}

export function showOnDirtyErrorStateMatcher(control: FormControl,
Copy link
Contributor

Choose a reason for hiding this comment

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

docs

@kara 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 tinayuangao merged commit f73cc97 into angular:master Jun 28, 2017
@willshowell willshowell deleted the feat/custom-error-function branch June 28, 2017 01:19
amcdnl pushed a commit to amcdnl/material2 that referenced this pull request Jul 8, 2017
* 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
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to customize when md-error is shown
6 participants