X Tutup
Skip to content

feat(forms): implement setErrors#4917

Closed
vsavkin wants to merge 1 commit intoangular:masterfrom
vsavkin:set_errors2
Closed

feat(forms): implement setErrors#4917
vsavkin wants to merge 1 commit intoangular:masterfrom
vsavkin:set_errors2

Conversation

@vsavkin
Copy link
Copy Markdown
Contributor

@vsavkin vsavkin commented Oct 26, 2015

Example:

var login = new Control("someLogin");
c.setErrors({"notUnique": true});
expect(c.valid).toEqual(false);
expect(c.errors).toEqual({"notUnique": true});

c.updateValue("newLogin");
expect(c.valid).toEqual(true);

BREAKING CHANGE:

Before:

ControlGroup.errors and ControlArray.errors returned the reduced values of their children controls' errors.

After:

ControlGroup.errors and ControlArray.errors return the errors of the group and array.
And ControlGroup.controlsErrors and ControlArray.controlsErrors return the reduce values of their children controls' errors.

Notes:

  • I had to separate errors and controlsErrors. This is to provide the following guarantee: A control group with invalid children is invalid.
  • Calling setErrors on a control will update the validity of its ancestors.

@vsavkin vsavkin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 26, 2015
mhevery
mhevery reviewed Oct 27, 2015
View reviewed changes
modules/angular2/src/core/forms/model.ts Outdated
Copy link
Copy Markdown
Contributor

@mhevery mhevery Oct 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add docs? It is easier to do now, then during a big docs push.

@mhevery mhevery added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 27, 2015
@vsavkin
feat(forms): Implement a way to manually set errors on a control
cf0251f 
Example:

var login = new Control("someLogin");
c.setErrors({"notUnique": true});
expect(c.valid).toEqual(false);
expect(c.errors).toEqual({"notUnique": true});

c.updateValue("newLogin");
expect(c.valid).toEqual(true);

BREAKING CHANGE:

Before:

ControlGroup.errors and ControlArray.errors returned a reduced value of their children controls' errors.

After:

ControlGroup.errors and ControlArray.errors return the errors of the group and array.
And ControlGroup.controlsErrors and ControlArray.controlsErrors return the reduce value of their children controls' errors.
@vsavkin vsavkin added the action: merge The PR is ready for merge by the caretaker label Oct 27, 2015
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Oct 27, 2015

Merging PR #4917 on behalf of @vsavkin to branch presubmit-vsavkin-pr-4917.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Oct 27, 2015
TedSander
TedSander reviewed Oct 27, 2015
View reviewed changes
modules/angular2/src/core/forms/model.ts
Copy link
Copy Markdown
Contributor

@TedSander TedSander Oct 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes sense to move group, and array from Validators? I'm worried that a user might try to pass them in the constructor, or pass them in as a combined validator. Should these functions be used as a regular validator at this point?

@TedSander
Copy link
Copy Markdown
Contributor

TedSander commented Oct 27, 2015

LGTM I like it turned out much cleaner than the original design.

@vsavkin vsavkin closed this in ed4826b Oct 27, 2015
@Krustie101
Copy link
Copy Markdown

Krustie101 commented Oct 27, 2015

I notice the following things in the source code:

  • The group/array validator only take into account the errors, not the controlErrors of the abstract controls in the group/array. Since the group/array validator are used to compute the controlErrors of the group/array, I expect the validy status of the group to be incorrect in the following case:
    control A in controlGroup B in controlGroup C
    Assume control A is invalid (either by internal validation or manual validation) and that we do not set the errors object of either B or C.
    The controlGroup B is invalid because the controlErrors object of B contains the error of A, but controlGroup C is valid because the error of A is not "passed through" to C.
  • It is a good idea that setting the errors property of a control overrides the errors that arose from the validator? I presume the underlying reasoning is that the manual validation should not have been performed because the control contained a validator error so it does not make a difference.
    But what if you have a numeric input with a maxValue validator and there are some other controls in the form and depending on their values, a min value constrained kicks in. I might try to write a validator for this or I might conclude that a manual validation (before submit?) is more appropriate in this case. Then any max value error (message) is lost if the min value constraint is not satisfied.
    Not overriding the errors arising from the validator in the control case (combined with clearing of manual errors if the value is updated) would also be more consistent with the group/array behaviour.
    Even simpler, a developer can clear all the errors (validator & manual) of a control by calling setError(null). This seems strange.

@Krustie101 Krustie101 mentioned this pull request Oct 29, 2015
Closed
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 7, 2019

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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vsavkin @mary-poppins @TedSander @Krustie101 @mhevery @googlebot
X Tutup