Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat(formController): add $setUntouched to propagate untouched state #9050

Closed
wants to merge 1 commit into from
Closed

feat(formController): add $setUntouched to propagate untouched state #9050

wants to merge 1 commit into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Sep 12, 2014

No description provided.

@tbosch
Copy link
Contributor

tbosch commented Sep 12, 2014

Do you have a usecase for this?

@shahata
Copy link
Contributor Author

shahata commented Sep 12, 2014

Yes, it is described in the comments of #583 (comment)
In a nutshell, it makes a lot of sense to call this when setting the form to pristine state, otherwise empty inputs with required validator will still have errors.

@btford btford added this to the 1.3.0-rc.3 milestone Sep 18, 2014
@caitp
Copy link
Contributor

caitp commented Sep 22, 2014

apparently I'm the "person" for this PR, despite not being the assignee! So I guess I'll take a look at this one

@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.3, 1.3.0-rc.4 Sep 22, 2014
@caitp caitp self-assigned this Sep 23, 2014
@caitp
Copy link
Contributor

caitp commented Sep 23, 2014

Yes, it is described in the comments of #583 (comment)
In a nutshell, it makes a lot of sense to call this when setting the form to pristine state, otherwise empty inputs with required validator will still have errors.

If that's the use case, is there a reason this use case is not implemented in this PR?

I'm not really wild about adding yet another method to the form/ngModel controllers, because there are already so many of them. Why not just make it a side effect of $setPristine() to also unset the $touched flag of each control?

@shahata
Copy link
Contributor Author

shahata commented Sep 23, 2014

It's fine with me to do this in setPristine. I can update this pr if you
prefer it this way. I originally thought it would be better to have
setUntouched in order to be consistent with ngModelController. Maybe we
should also update setPristine in ngModelController to setUntouched?
On Sep 23, 2014 2:24 PM, "Caitlin Potter" notifications@github.com wrote:

Yes, it is described in the comments of #583
#583 (comment)
In a nutshell, it makes a lot of sense to call this when setting the form
to pristine state, otherwise empty inputs with required validator will
still have errors.

If that's the use case, is there a reason this use case is not implemented
in this PR?


Reply to this email directly or view it on GitHub
#9050 (comment).

@caitp
Copy link
Contributor

caitp commented Sep 23, 2014

It's probably not a good idea to rename things that are already exposed ---- another opinion on this would be good, but personally I'm not keen on adding yet another one, especially if its usage is supposed to be tied to setPristine

@shahata
Copy link
Contributor Author

shahata commented Sep 23, 2014

I agree with you that it makes sense to unset the $touched state in $setPristine(). I'm just saying that maybe we should do the same in ngModelController.$setPristine() and not only in FormController.$setPristine in order to be consistent. I don't mean that we should rename anything, just update the behavior of ngModelController.$setPrisitine().

@tbosch
Copy link
Contributor

tbosch commented Sep 23, 2014

Let's leave the PR as it is:
Right now, $setPristine and $setUntouched are separated already in ngModelController. So having them separate in ngModelController is consistent. Don't want to introduce breaking changes in the RC phase if not needed...

@tbosch
Copy link
Contributor

tbosch commented Sep 23, 2014

Ok, LGTM.

@caitp
Copy link
Contributor

caitp commented Sep 23, 2014

@shahata do you think this fully closes #583? or is there more to that issue than this. It seems pretty vague

@caitp
Copy link
Contributor

caitp commented Sep 23, 2014

checks if any other issues have been filed about that

@caitp caitp closed this in fd89975 Sep 23, 2014
@shahata
Copy link
Contributor Author

shahata commented Sep 23, 2014

Yeah, I think it's time to close #583 :)
On Sep 23, 2014 8:51 PM, "Caitlin Potter" notifications@github.com wrote:

@shahata https://github.com/shahata do you think this fully closes #583
#583? or is there more to
that issue than this. It seems pretty vague


Reply to this email directly or view it on GitHub
#9050 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants