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

feat(forms): add markAllAsTouched() to AbstractControl #26812

Closed

Conversation

@alsami
Copy link
Contributor

alsami commented Oct 29, 2018

Add functionality to mark a control and its descendant controls as touched

Closes #19400

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

#19400

Issue Number: #19400

What is the new behavior?

Allows an abstract control and its descendants to be marked as touched

Does this PR introduce a breaking change?

  • Yes
  • No

@Splaktar as discussed marking you in this PR.

@googlebot googlebot added the cla: yes label Oct 29, 2018

@alsami alsami changed the title feat(forms): add `markAllAsTouched()` to `AbstractControl` WIP: feat(forms): add `markAllAsTouched()` to `AbstractControl` Oct 29, 2018

@alsami alsami changed the title WIP: feat(forms): add `markAllAsTouched()` to `AbstractControl` WIP: feat(forms): add markAllAsTouched() to AbstractControl Oct 29, 2018

@alsami

This comment has been minimized.

Copy link
Contributor Author

alsami commented Oct 29, 2018

I am wondering whether AbstractControl is the right place for this feature or not. Maybe it makes more sense to only make this available for FormGroup?

@alsami alsami force-pushed the alsami:feature-forms-mark-all-as-touched branch 3 times, most recently from cbd8541 to 5c39d05 Oct 29, 2018

@Splaktar
Copy link
Member

Splaktar left a comment

I had a couple of minor comments, but otherwise this looks very good to me!

Show resolved Hide resolved packages/forms/src/model.ts Outdated
Show resolved Hide resolved packages/forms/test/form_group_spec.ts Outdated
@Splaktar

This comment has been minimized.

Copy link
Member

Splaktar commented Oct 30, 2018

It looks like some relevant tests may need to be added to the following as well

It looks like no docs are needed as the Forms Guides don't cover any of the markAs* functions and the API docs are auto generated from the comments.

@Splaktar

This comment has been minimized.

Copy link
Member

Splaktar commented Oct 30, 2018

  • You'll want to squash your commits.
Show resolved Hide resolved packages/forms/src/model.ts Outdated

@alsami alsami force-pushed the alsami:feature-forms-mark-all-as-touched branch 2 times, most recently from 8cad1f4 to 156b2c9 Oct 30, 2018

@alsami

This comment has been minimized.

Copy link
Contributor Author

alsami commented Oct 30, 2018

  • You'll want to squash your commits.

Commits squashed. Will do so for any further commits.

@alsami alsami closed this Oct 30, 2018

@alsami alsami reopened this Oct 30, 2018

@alsami alsami force-pushed the alsami:feature-forms-mark-all-as-touched branch from 156b2c9 to ac78c16 Oct 30, 2018

@Splaktar

This comment has been minimized.

Copy link
Member

Splaktar commented Oct 30, 2018

Note that if you amend your previous commit and force push, you can avoid having to do interactive rebase to squash commits 😄

@alsami alsami force-pushed the alsami:feature-forms-mark-all-as-touched branch from 4ceb3fb to 7a29a6e Nov 12, 2018

@alxhub alxhub added the comp: forms label Dec 11, 2018

@ngbot ngbot bot added this to the needsTriage milestone Dec 11, 2018

@kara
Copy link
Contributor

kara left a comment

The change itself looks good, but we need tests for calling the method in FormArray and FormControl in their respective test files (e.g. myArr.markAllAsTouched() and control.markAllAsTouched())

Show resolved Hide resolved packages/forms/test/form_group_spec.ts Outdated

@kara kara added the aio: preview label Dec 13, 2018

@kara

kara approved these changes Dec 28, 2018

Copy link
Contributor

kara left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Dec 28, 2018

@IgorMinar Can you review for public API changes?

@kara kara requested a review from IgorMinar Dec 28, 2018

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Dec 28, 2018

@alsami

This comment has been minimized.

Copy link
Contributor Author

alsami commented Jan 10, 2019

@kara should I fix the merge conflict or can @IgorMinar do that when approving API changes?

@IgorMinar
Copy link
Member

IgorMinar left a comment

Lgtm for api but please fix the docs.

Show resolved Hide resolved packages/forms/src/model.ts Outdated

@alsami alsami force-pushed the alsami:feature-forms-mark-all-as-touched branch from 0b7b7a5 to ad553ea Jan 10, 2019

@alsami alsami requested review from angular/fw-forms as code owners Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

feat(forms): add `markAllAsTouched()` to `AbstractControl`
Add functionality to mark a control and its descendant controls as touched

Closes #19400

@alsami alsami force-pushed the alsami:feature-forms-mark-all-as-touched branch from ad553ea to 02c771f Jan 12, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 12, 2019

@alsami

This comment has been minimized.

Copy link
Contributor Author

alsami commented Jan 12, 2019

@kara I fixed the merge conflicts and rebased on master. Docs are looking fine now. If there is more left to be done please let me know.

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Jan 18, 2019

@alsami Should be good to go! Applied the merge label.

@alsami

This comment has been minimized.

Copy link
Contributor Author

alsami commented Jan 18, 2019

Took us a while, thanks for the help!

@alxhub alxhub closed this in 45bf911 Jan 18, 2019

@alsami alsami deleted the alsami:feature-forms-mark-all-as-touched branch Jan 21, 2019

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

feat(forms): add `markAllAsTouched()` to `AbstractControl` (angular#2…
…6812)

Add functionality to mark a control and its descendant controls as touched

Closes angular#19400

PR Close angular#26812

@damienwebdev damienwebdev referenced this pull request Feb 12, 2019

Merged

Design input component #248

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.