feat(forms): Added emitEvent to AbstractControl methods #11949

Merged
merged 2 commits into from Oct 19, 2016

Conversation

Projects
None yet
5 participants
@Fank
Contributor

Fank commented Sep 27, 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 #11286
Fixes #11551

What is the new behavior?

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:
I haven't added test because i checked the spec files for FormArray, FormGroup and both did not contain the current supported "option" onlySelf so, if these aren't tested, there is also no need for emitEvent. Same behavior for the docs.

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

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Sep 27, 2016

Member

please separate your changes:

  • style(forms): ... -> style changes
  • fix(forms): ... -> actual fix
Member

vicb commented Sep 27, 2016

please separate your changes:

  • style(forms): ... -> style changes
  • fix(forms): ... -> actual fix
@Fank

This comment has been minimized.

Show comment
Hide comment
@Fank

Fank Sep 28, 2016

Contributor

@vicb #11948 (comment) thanks for the info, i haven't see the information about logical separating commits, in https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit . But yea you are right, it makes it much easier to read. Fixed it.

Contributor

Fank commented Sep 28, 2016

@vicb #11948 (comment) thanks for the info, i haven't see the information about logical separating commits, in https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit . But yea you are right, it makes it much easier to read. Fixed it.

@Fank

This comment has been minimized.

Show comment
Hide comment
@Fank

Fank Sep 28, 2016

Contributor

/cc @kara

Contributor

Fank commented Sep 28, 2016

/cc @kara

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Oct 10, 2016

Contributor

@Fank Thanks for the PR! Can you please add tests for the methods that have changed?

Contributor

kara commented Oct 10, 2016

@Fank Thanks for the PR! Can you please add tests for the methods that have changed?

@Fank

This comment has been minimized.

Show comment
Hide comment
@Fank

Fank Oct 10, 2016

Contributor

@kara as i wrote above in "Other information", there is current no specs for those cases.
How do should i test it?
And should i added test for onlySelf too?

Contributor

Fank commented Oct 10, 2016

@kara as i wrote above in "Other information", there is current no specs for those cases.
How do should i test it?
And should i added test for onlySelf too?

@kara

This comment has been minimized.

Show comment
Hide comment
@kara

kara Oct 10, 2016

Contributor

@Fank Adding tests for the code you changed is standard when submitting PRs. The form control, form group, and form array specs would be the appropriate places. Feel free to add tests for onlySelf as well if you feel that would improve our coverage.

Contributor

kara commented Oct 10, 2016

@Fank Adding tests for the code you changed is standard when submitting PRs. The form control, form group, and form array specs would be the appropriate places. Feel free to add tests for onlySelf as well if you feel that would improve our coverage.

@Fank

This comment has been minimized.

Show comment
Hide comment
@Fank

Fank Oct 10, 2016

Contributor

@kara sry for the 1. question, it was miss written i mean: (im very sorry about that "angry" comment)
How do the test should looks like? Because i don't how to test it, like https://github.com/angular/angular/blob/master/modules/%40angular/forms/test/form_control_spec.ts#L338 for every method?

I'm at angular days in germany right now and confused a little bit.

Contributor

Fank commented Oct 10, 2016

@kara sry for the 1. question, it was miss written i mean: (im very sorry about that "angry" comment)
How do the test should looks like? Because i don't how to test it, like https://github.com/angular/angular/blob/master/modules/%40angular/forms/test/form_control_spec.ts#L338 for every method?

I'm at angular days in germany right now and confused a little bit.

@Fank

This comment has been minimized.

Show comment
Hide comment
@Fank

Fank Oct 11, 2016

Contributor

@kara DONE, i'll do the onlySelf in a seperate PR. Added tests for the emitEvent change.
Found a test in the FormControl, which i used for every test.

Contributor

Fank commented Oct 11, 2016

@kara DONE, i'll do the onlySelf in a seperate PR. Added tests for the emitEvent change.
Found a test in the FormControl, which i used for every test.

@kara

Thanks for doing the tests! The FormArray and FormGroup tests need some updates, then we should be good to go.

+ c.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+ c2.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+
+ c.setValue('newValue', {emitEvent: false});

This comment has been minimized.

@kara

kara Oct 12, 2016

Contributor

The array spec should be testing the FormArray's setValue method, so instead call:

a.setValue(['one', 'two'], {emitEvent: false});
@kara

kara Oct 12, 2016

Contributor

The array spec should be testing the FormArray's setValue method, so instead call:

a.setValue(['one', 'two'], {emitEvent: false});
+ c.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+ c2.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+
+ c.patchValue('newValue', {emitEvent: false});

This comment has been minimized.

@kara

kara Oct 12, 2016

Contributor

Similarly, this should call a.patchValue

@kara

kara Oct 12, 2016

Contributor

Similarly, this should call a.patchValue

+ c2.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+ c3.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+
+ c.reset({}, {emitEvent: false});

This comment has been minimized.

@kara

kara Oct 12, 2016

Contributor

a.reset here (with an array as the first arg)

@kara

kara Oct 12, 2016

Contributor

a.reset here (with an array as the first arg)

+ c.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+ c2.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+
+ c.reset({}, {emitEvent: false});

This comment has been minimized.

@kara

kara Oct 12, 2016

Contributor

I'd reset the value to null here (first arg), as you're not really testing the value

@kara

kara Oct 12, 2016

Contributor

I'd reset the value to null here (first arg), as you're not really testing the value

+ g.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+ c.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+
+ c.setValue('newValue', {emitEvent: false});

This comment has been minimized.

@kara

kara Oct 12, 2016

Contributor

As this is the FormGroup spec, you'll want to test the FormGroups' setValue method.

g.setValue({'one': 'one', 'two': 'two'}, {emitEvent: false});
@kara

kara Oct 12, 2016

Contributor

As this is the FormGroup spec, you'll want to test the FormGroups' setValue method.

g.setValue({'one': 'one', 'two': 'two'}, {emitEvent: false});
+ g.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+ c.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+
+ c.patchValue('newValue', {emitEvent: false});

This comment has been minimized.

@kara

kara Oct 12, 2016

Contributor

g.patchValue

@kara

kara Oct 12, 2016

Contributor

g.patchValue

+ g.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+ c.valueChanges.subscribe((value) => { throw 'Should not happen'; });
+
+ c.reset({}, {emitEvent: false});

This comment has been minimized.

@kara

kara Oct 12, 2016

Contributor

g.reset

@kara

kara Oct 12, 2016

Contributor

g.reset

Fank added some commits Oct 11, 2016

@Fank

This comment has been minimized.

Show comment
Hide comment
@Fank

Fank Oct 14, 2016

Contributor

@kara done

Contributor

Fank commented Oct 14, 2016

@kara done

@Fank Fank referenced this pull request Oct 14, 2016

Merged

test(forms): Added missing selfOnly tests #12317

3 of 3 tasks complete
@kara

kara approved these changes Oct 17, 2016

LGTM! Thanks again for the PR.

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

2 of 3 checks passed

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

@Fank Fank deleted the Fank:fk_forms_emitvalue branch Oct 20, 2016

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

feat(forms): Added emitEvent to AbstractControl methods (#11949)
* feat(forms): Added emitEvent to AbstractControl methods

* style(forms): unified named parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment