-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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): consistent error behavior to md-input-container #4754
Conversation
src/lib/select/select.spec.ts
Outdated
@Component({ | ||
template: ` | ||
<form #form="ngForm" novalidate> | ||
<md-select placeholder="Food" [formControl]="formControl"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're trying to test NgForm
, it'd be more idiomatic to use it with ngModel
here, as formControl
is from the other forms module and it's not recommended to mix them. Can you switch to ngModel
so the tests more closely match common use cases?
Also, novalidate
isn't necessary here because it's added automatically by NgNoValidate
.
<form>
<md-select placeholder="Food" name="food" [ngModel]="food" required>
...
</md-select>
</form>
src/lib/select/select.spec.ts
Outdated
|
||
@Component({ | ||
template: ` | ||
<form [formGroup]="formGroup" novalidate> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, novalidate
should be added automatically
src/lib/select/select.spec.ts
Outdated
` | ||
}) | ||
class SelectInsideForm { | ||
@ViewChild('form') form: NgForm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you're not using @ViewChild(NgForm) form: NgForm
here instead? That way, you don't have to export the string.
src/lib/select/select.ts
Outdated
@@ -533,6 +536,17 @@ export class MdSelect implements AfterContentInit, OnDestroy, OnInit, ControlVal | |||
this._setScrollTop(); | |||
} | |||
|
|||
/** Whether the select is in an error state. */ | |||
_isErrorState(): boolean { | |||
const control = this._control; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: seems like you could inline this, given how short it is.
3d8c223
to
cf4ec30
Compare
Rebased and addressed the feedback @kara. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but needs rebase again. Apply label when ready.
cf4ec30
to
15be464
Compare
Rebase please |
* Gets `md-select` to behave in the same way as `md-input-container` when it comes to errors. This means highlighting itself when it is invalid and touched, or one of the parent forms/form groups is submitted. * Moves the error state logic into a separate function in order to avoid some hard-to-follow selectors and to potentially allow overrides. This should also be a first step to supporting `md-error` inside `md-select`. * Changes the required asterisk to always have the theme warn color, similarly to the input asterisk. Fixes angular#4611.
15be464
to
57f5f7c
Compare
+1 |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
md-select
to behave in the same way asmd-input-container
when it comes to errors. This means highlighting itself when it is invalid and touched, or one of the parent forms/form groups is submitted.md-error
insidemd-select
.Fixes #4611.