feat(forms): add markAllAsDirty to AbstractControl #58663

aberonni · 2024-11-14T16:17:47Z

Adds the markAllAsDirty method to the AbstractControl class. This method will mark the control and all its descendants as dirty.

I pretty much just duplicated the behaviour and tests of markAllAsTouched.

Fixes #55990

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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.dev application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #55990

What is the new behavior?

Adding markAllAsDirty to AbstractControl

Does this PR introduce a breaking change?

Yes
No

Other information

N/A

Adds the `markAllAsDirty` method to the `AbstractControl` class. This method will mark the control and all its descendants as dirty. I pretty much just duplicated the behaviour and tests of `markAllAsTouched`. Fixes angular#55990

aberonni · 2024-12-31T16:44:52Z

@AndrewKushnir just checking if this requires any action from me?

packages/forms/src/model/abstract_model.ts

JeanMeche · 2025-01-01T17:34:08Z

packages/forms/test/form_array_spec.ts

+
+        expect(innerFormArrayGroupControl1.dirty).toBe(false);
+
+        formArray.markAllAsDirty();


Can we add tests that also cover emitEvent ? (and checking the events are emitted)

I've added a basic test which I grabbed from elsewhere for inspiration. I also did the same for markAllAsTouched which is what I got inspiration from for the other tests.

michael-small · 2025-01-01T17:37:33Z

packages/forms/test/form_array_spec.ts

+
+        expect(group1.dirty).toBe(false);
+
+        const group1Control1 = group1.get('c1') as FormControl;


This question may be bigger than this PR's scope:

I see that the old fashioned .get('...') as FormXXX conventions come from existing tests. But I wonder: is there any reason for this now other than consistency? I would think they could be properly typed now by just accessing the now default typing. Either just for these new tests now, or perhaps someone take a more formal sweep through form tests in a separate PR.

Tbh I just followed what was in the tests for markAllAsTouched. This is my first contribution to angular so I'm not sure how else to do things, but happy to adjust if this is deemed within the scope of my contribution.

github-actions · 2025-01-01T17:39:29Z

Deployed adev-preview for 587a1c8 to: https://ng-dev-previews-fw--pr-angular-angular-58663-adev-prev-qaaol9d0.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

Co-authored-by: michael-small <[email protected]>

michael-small

LGTM

kirjs · 2025-01-03T21:21:01Z

packages/forms/test/form_group_spec.ts

@@ -141,6 +221,12 @@ import {StatusChangeEvent} from '../src/model/abstract_model';

        expect(arrayFirstChildGroupFirstChildCtrl.touched).toBe(true);
      });
+
+      it('should not emit events when `FormGroup.markAllAsTouched` is called with `emitEvent: false`', () => {


Thanks for adding this test.
Consider also adding a test that verifies that event is emitted for true and not having a value.

kirjs · 2025-01-03T21:27:37Z

packages/forms/src/model/abstract_model.ts

+   * When false, no events are emitted.
+   */
+  markAllAsDirty(opts: {emitEvent?: boolean} = {}): void {
+    this.markAsDirty({onlySelf: true, emitEvent: opts.emitEvent, sourceControl: this});


I'm wondering why onlySelf is not being passed as a prop to cover the case where we want to mark descendants but not ascendance?

I assume this is just something you copypasted from markAllAsTouched, but curious if you have any context for this.

kirjs · 2025-01-03T21:36:19Z

packages/forms/test/form_group_spec.ts

+        expect(arrayFirstChildGroupFirstChildCtrl.dirty).toBe(true);
+      });
+
+      it('should not emit events when `FormGroup.markAllAsDirty` is called with `emitEvent: false`', () => {


Please add a test that verifies that event is emitted for true and not having a value.

kirjs · 2025-01-03T21:39:52Z

packages/forms/src/model/abstract_model.ts

+   *
+   * @param opts Configuration options that determine how the control propagates changes
+   * and emits events after marking is applied.
+   * * `emitEvent`: When true or not supplied (the default), the `events`


Looking at the code, I'm not convinced that true and missing value would do the same thing, but a test would help clarify that

undefined is !== false.
I think we're good here !

aberonni · 2025-01-20T16:07:15Z

@kirjs thanks for the review. I'm not sure how to proceed on this, given that the PR seems to have the necessary approvals, but there are also change requests. I also want to point out that the requested changes seem to be beyond the scope of my addition - given that they are addressing feedback on tests that were already there. If it helps get this merged, I can put more time and effort in, but I doubt I am the best suited to put that effort in (I'm a first-time contributor, and just trying to add a very small change).

aberonni · 2025-02-03T16:33:24Z

@AndrewKushnir @kirjs @michael-small can you let me know if this needs any action from me or whether we can proceed as is?

michael-small · 2025-02-03T16:37:53Z

I'm just a random community guy who keeps up with forms PRs. I think these changes are legit. My comments were just thinking out loud to real maintainers about the existing tests beyond the scope of your PR.

Looks good to me for what it's worth

JeanMeche · 2025-02-03T16:42:07Z

Pullapprove is reporting that we miss 2 public-api scope approvals.

angular-robot bot added detected: feature PR contains a feature commit area: forms labels Nov 14, 2024

ngbot bot added this to the Backlog milestone Nov 14, 2024

aberonni marked this pull request as ready for review November 14, 2024 16:19

pullapprove bot requested a review from AndrewKushnir November 14, 2024 16:19

feat(forms): add markAllAsDirty to AbstractControl

587a1c8

Adds the `markAllAsDirty` method to the `AbstractControl` class. This method will mark the control and all its descendants as dirty. I pretty much just duplicated the behaviour and tests of `markAllAsTouched`. Fixes angular#55990

aberonni force-pushed the aberonni/add-markAllAsDirty branch from 250f823 to 587a1c8 Compare November 14, 2024 16:22

AndrewKushnir modified the milestones: Backlog, v19.1 candidates Nov 14, 2024

angular deleted a comment from google-cla bot Dec 16, 2024

michael-small reviewed Jan 1, 2025

View reviewed changes

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

JeanMeche added the adev: preview label Jan 1, 2025

JeanMeche reviewed Jan 1, 2025

View reviewed changes

michael-small reviewed Jan 1, 2025

View reviewed changes

Update packages/forms/src/model/abstract_model.ts

6ce833f

Co-authored-by: michael-small <[email protected]>

aberonni force-pushed the aberonni/add-markAllAsDirty branch from 0f78d85 to 6ce833f Compare January 2, 2025 08:50

Add tests for emitEvent: false

e30c1c8

aberonni requested review from michael-small and JeanMeche January 2, 2025 09:52

michael-small approved these changes Jan 3, 2025

View reviewed changes

kirjs approved these changes Jan 3, 2025

View reviewed changes

pullapprove bot requested review from alxhub and pkozlowski-opensource January 3, 2025 21:36

kirjs reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(forms): add markAllAsDirty to AbstractControl #58663

feat(forms): add markAllAsDirty to AbstractControl #58663

aberonni commented Nov 14, 2024

aberonni commented Dec 31, 2024

JeanMeche Jan 1, 2025 •

edited

Loading

aberonni Jan 2, 2025

michael-small Jan 1, 2025 •

edited

Loading

aberonni Jan 2, 2025

github-actions bot commented Jan 1, 2025

michael-small left a comment

kirjs Jan 3, 2025

kirjs Jan 3, 2025

kirjs Jan 3, 2025

kirjs Jan 3, 2025

JeanMeche Feb 3, 2025

aberonni commented Jan 20, 2025

aberonni commented Feb 3, 2025

michael-small commented Feb 3, 2025 •

edited

Loading

JeanMeche commented Feb 3, 2025


		expect(innerFormArrayGroupControl1.dirty).toBe(false);

		formArray.markAllAsDirty();


		expect(group1.dirty).toBe(false);

		const group1Control1 = group1.get('c1') as FormControl;

feat(forms): add markAllAsDirty to AbstractControl #58663

Are you sure you want to change the base?

feat(forms): add markAllAsDirty to AbstractControl #58663

Conversation

aberonni commented Nov 14, 2024

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

aberonni commented Dec 31, 2024

JeanMeche Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

aberonni Jan 2, 2025

Choose a reason for hiding this comment

michael-small Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

aberonni Jan 2, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2025

michael-small left a comment

Choose a reason for hiding this comment

kirjs Jan 3, 2025

Choose a reason for hiding this comment

kirjs Jan 3, 2025

Choose a reason for hiding this comment

kirjs Jan 3, 2025

Choose a reason for hiding this comment

kirjs Jan 3, 2025

Choose a reason for hiding this comment

JeanMeche Feb 3, 2025

Choose a reason for hiding this comment

aberonni commented Jan 20, 2025

aberonni commented Feb 3, 2025

michael-small commented Feb 3, 2025 • edited Loading

JeanMeche commented Feb 3, 2025

JeanMeche Jan 1, 2025 •

edited

Loading

michael-small Jan 1, 2025 •

edited

Loading

michael-small commented Feb 3, 2025 •

edited

Loading