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

fix(select): errors not shown on submit #7640

Merged
merged 1 commit into from
Oct 9, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Oct 8, 2017

Fixes mat-select error messages not being shown when the parent form is submitted.

Fixes #7634.

@mmalerba this is more or less what the input does already. I was considering moving it into the MatFormFieldControl, but having lifecycle hooks in there might look weird. What do you think?

Fixes `mat-select` error messages not being shown when the parent form is submitted.

Fixes angular#7634.
@crisbeto crisbeto requested a review from kara as a code owner October 8, 2017 17:01
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 8, 2017
@mmalerba
Copy link
Contributor

mmalerba commented Oct 9, 2017

I was thinking we should just move all the ErrorStateMatcher stuff into mat-form-field anyways. It's essentially all the same logic duplicated between input and select, and anyone who implements a custom control is probably going to want similar behavior. (but also fine if you just want to do this for now to fix select)

@crisbeto
Copy link
Member Author

crisbeto commented Oct 9, 2017

My issue with having it there is that some bindings on the form field control depend on it (e.g. the aria-invalid on the input). We'd have to introduce another subject like stateChanges, but on the form field, which could make things even more convoluted.

@mmalerba
Copy link
Contributor

mmalerba commented Oct 9, 2017

Hmmm I see, what if we changed to, instead of an @Input to a @Directive: [matErrorStateMatcher] which would basically do this:

@Directive({
  ...
  providers: [{provide: ErrorStateMatcher, useExisting: MatErrorStateMatcherDirective}]
})
export class MatErrorStateMatcherDirective implements ErrorStateMatcher {
  @Input('matErrorStateMatcher') errorStateMatcher: ErrorStateMatcher;

  isErrorState(...) {
    return this.errorStateMatcher.isErrorState(...);
  }
}

This way form field and input could both just inject the token and not worry about dealing with the @Input. It also has a cool side-effect of being able to do this:

<form class="my-special-error-group" [matErrorStateMatcher]="myMatcher">
  <!-- bunch of form fields that all get my custom matcher -->
</form>

@crisbeto
Copy link
Member Author

crisbeto commented Oct 9, 2017

I don't think that would react to changes either. Angular marks it for checking if the @Input value changes. Since the input is the error matcher itself, it won't do anything when the error state changes.

@mmalerba
Copy link
Contributor

mmalerba commented Oct 9, 2017

Ok then, let's do this for now, I'll keep thinking if there's any good way to move this common logic somewhere else...

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Oct 9, 2017
@andrewseguin andrewseguin merged commit d2f41a4 into angular:master Oct 9, 2017
@listepo-alterpost
Copy link

@crisbeto when will this fix be released?

@harshi1991
Copy link

Is this fix released?

@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 8, 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.

mat-select doesn't show mat-error on submit
6 participants