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): consistent error behavior to md-input-container #4754

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

crisbeto
Copy link
Member

  • 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 #4611.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 23, 2017
@Component({
template: `
<form #form="ngForm" novalidate>
<md-select placeholder="Food" [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.

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>


@Component({
template: `
<form [formGroup]="formGroup" novalidate>
Copy link
Contributor

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

`
})
class SelectInsideForm {
@ViewChild('form') form: NgForm;
Copy link
Contributor

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.

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

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.

@kara kara assigned crisbeto and unassigned kara May 31, 2017
@crisbeto crisbeto force-pushed the 4611/select-error-state branch 2 times, most recently from 3d8c223 to cf4ec30 Compare May 31, 2017 05:16
@crisbeto
Copy link
Member Author

Rebased and addressed the feedback @kara.

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.

LGTM, but needs rebase again. Apply label when ready.

@kara kara assigned crisbeto and unassigned kara Jul 21, 2017
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jul 22, 2017
@andrewseguin
Copy link
Contributor

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.
@finalxcode
Copy link

+1

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

invalid MdSelect should highlight itself on parent form submitted
5 participants