feat(forms): add hasError and getError to AbstractControlDirective #11985

Merged
merged 1 commit into from Oct 19, 2016

Conversation

Projects
None yet
5 participants
@cexbrayat
Contributor

cexbrayat commented Sep 29, 2016

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Fixes #7255

Allows cleaner expressions in template-driven forms.

Current behavior:

<label>Username</label><input name="username" ngModel required #username="ngModel">
<div *ngIf="username.dirty && username.control.hasError('required')">Username is required</div>

What is the new behavior?

New behavior:

<label>Username</label><input name="username" ngModel required #username="ngModel">
<div *ngIf="username.dirty && username.hasError('required')">Username is required</div>

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[X] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@googlebot googlebot added the cla: yes label Sep 29, 2016

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Sep 29, 2016

Member

Please add tests and fix the CI

Member

vicb commented Sep 29, 2016

Please add tests and fix the CI

@cexbrayat

This comment has been minimized.

Show comment
Hide comment
@cexbrayat

cexbrayat Sep 29, 2016

Contributor

@vicb (hopefully) done

Contributor

cexbrayat commented Sep 29, 2016

@vicb (hopefully) done

@@ -511,6 +511,16 @@ export function main() {
expect(ngModel.enabled).toBe(control.enabled);
});
+ it('should reexport control methods', () => {
+ const control = ngModel.control;

This comment has been minimized.

@vicb

vicb Sep 29, 2016

Member

expect(ngModel.hasError('required')).toEqual(control.hasError('required'); ?

@vicb

vicb Sep 29, 2016

Member

expect(ngModel.hasError('required')).toEqual(control.hasError('required'); ?

This comment has been minimized.

@cexbrayat

cexbrayat Sep 29, 2016

Contributor

@vicb amended the commit with your suggestion

@cexbrayat

cexbrayat Sep 29, 2016

Contributor

@vicb amended the commit with your suggestion

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Sep 29, 2016

Member

@kara will review your PR next week. Note that this could only be merged in 2.1 (API addition)

Member

vicb commented Sep 29, 2016

@kara will review your PR next week. Note that this could only be merged in 2.1 (API addition)

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Oct 10, 2016

Contributor

@cexbrayat Thanks for the PR! Looks good, but since you're adding the methods to AbstractControlDirective, you'll need tests for all abstract control directives (not just ngModel). Can you add parallel tests for other directives affected?

Contributor

kara commented Oct 10, 2016

@cexbrayat Thanks for the PR! Looks good, but since you're adding the methods to AbstractControlDirective, you'll need tests for all abstract control directives (not just ngModel). Can you add parallel tests for other directives affected?

@cexbrayat

This comment has been minimized.

Show comment
Hide comment
@cexbrayat

cexbrayat Oct 10, 2016

Contributor

@kara maybe it would be simpler to add a test suite for a DummyAbstractControlDirective ? It's strange to test all the classes that inherits an abstract class, just to test the base class methods...
What do you think ?

Contributor

cexbrayat commented Oct 10, 2016

@kara maybe it would be simpler to add a test suite for a DummyAbstractControlDirective ? It's strange to test all the classes that inherits an abstract class, just to test the base class methods...
What do you think ?

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Oct 10, 2016

Contributor

I think it's odder to test just one of the directives that your change affects. Fair point regarding a test suite, but for now I'd prefer to keep each directive's tests explicit so it's clear what each supports.

Contributor

kara commented Oct 10, 2016

I think it's odder to test just one of the directives that your change affects. Fair point regarding a test suite, but for now I'd prefer to keep each directive's tests explicit so it's clear what each supports.

@cexbrayat

This comment has been minimized.

Show comment
Hide comment
@cexbrayat

cexbrayat Oct 10, 2016

Contributor

@kara I attempted to refactor the test, to avoid the copy/pasted assertions in every test, and introduced a test of the base class. Let me know if you like it (if not, I'll add the test for every directive).

Contributor

cexbrayat commented Oct 10, 2016

@kara I attempted to refactor the test, to avoid the copy/pasted assertions in every test, and introduced a test of the base class. Let me know if you like it (if not, I'll add the test for every directive).

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Oct 12, 2016

Contributor

@cexbrayat As mentioned, I really do prefer testing the directives explicitly. The existing test structure should be sufficient, so please add your tests on top. Thanks!

Contributor

kara commented Oct 12, 2016

@cexbrayat As mentioned, I really do prefer testing the directives explicitly. The existing test structure should be sufficient, so please add your tests on top. Thanks!

feat(forms): add hasError and getError to AbstractControlDirective
Allows cleaner expressions in template-driven forms.

Before:

    <label>Username</label><input name="username" ngModel required #username="ngModel">
    <div *ngIf="username.dirty && username.control.hasError('required')">Username is required</div>

After:

    <label>Username</label><input name="username" ngModel required #username="ngModel">
    <div *ngIf="username.dirty && username.hasError('required')">Username is required</div>

Fixes #7255
@cexbrayat

This comment has been minimized.

Show comment
Hide comment
@cexbrayat

cexbrayat Oct 13, 2016

Contributor

@kara As you prefer! Check the last commit with the updated tests.

Contributor

cexbrayat commented Oct 13, 2016

@kara As you prefer! Check the last commit with the updated tests.

@cexbrayat

This comment has been minimized.

Show comment
Hide comment
@cexbrayat

cexbrayat Oct 13, 2016

Contributor

@kara The failing test on CI looks unrelated to the PR...

Contributor

cexbrayat commented Oct 13, 2016

@kara The failing test on CI looks unrelated to the PR...

@kara

kara approved these changes Oct 14, 2016

LGTM! Agree that the failing test was unrelated. Probably a flake.

@alxhub alxhub merged commit 592f40a into angular:master Oct 19, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

btrigueiro added a commit to btrigueiro/angular that referenced this pull request Oct 21, 2016

feat(forms): add hasError and getError to AbstractControlDirective (#…
…11985)

Allows cleaner expressions in template-driven forms.

Before:

    <label>Username</label><input name="username" ngModel required #username="ngModel">
    <div *ngIf="username.dirty && username.control.hasError('required')">Username is required</div>

After:

    <label>Username</label><input name="username" ngModel required #username="ngModel">
    <div *ngIf="username.dirty && username.hasError('required')">Username is required</div>

Fixes #7255
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment